Clarify use of a clang-tidy check

The google-default-arguments check normally imposes that virtual
methods have no default arguments. For our use-cases, all
implementations of such methods are expected to use the same
default arguments, and thus this check is manually disabled via
NOLINT comments. However, this is not done consistently.

This commit cleans this up and clarifies our intent by:
- removing the default values (and the NOLINT statement) for all
  implementations of virtual methods with default argument values,
  matching the desired intended behaviour, but
- keeping the clang-tidy check for future cases where derived
  classes would want to provide each different defaults.

7 files changed, 29 insertions, 43 deletions

diff --git a/src/buildtool/execution_api/local/local_api.cpp b/src/buildtool/execution_api/local/local_api.cpp
index c0a3a30e..d746c8e9 100644
--- a/src/buildtool/execution_api/local/local_api.cpp
+++ b/src/buildtool/execution_api/local/local_api.cpp
@@ -59,7 +59,6 @@ LocalApi::LocalApi(gsl::not_null<LocalContext const*> const& local_context,
     : local_context_{*local_context},
       git_api_{CreateFallbackApi(*local_context->storage, repo_config)} {}
 
-// NOLINTNEXTLINE(google-default-arguments)
 auto LocalApi::CreateAction(
     ArtifactDigest const& root_digest,
     std::vector<std::string> const& command,
@@ -102,7 +101,6 @@ auto LocalApi::CreateAction(
                                                                 properties}};
 }
 
-// NOLINTNEXTLINE(google-default-arguments)
 auto LocalApi::RetrieveToPaths(
     std::vector<Artifact::ObjectInfo> const& artifacts_info,
     std::vector<std::filesystem::path> const& output_paths,
@@ -130,7 +128,6 @@ auto LocalApi::RetrieveToPaths(
     return true;
 }
 
-// NOLINTNEXTLINE(google-default-arguments)
 auto LocalApi::RetrieveToFds(
     std::vector<Artifact::ObjectInfo> const& artifacts_info,
     std::vector<int> const& fds,
diff --git a/src/buildtool/execution_api/local/local_api.hpp b/src/buildtool/execution_api/local/local_api.hpp
index 1b6ddeab..92761738 100644
--- a/src/buildtool/execution_api/local/local_api.hpp
+++ b/src/buildtool/execution_api/local/local_api.hpp
@@ -40,7 +40,6 @@ class LocalApi final : public IExecutionApi {
     explicit LocalApi(gsl::not_null<LocalContext const*> const& local_context,
                       RepositoryConfig const* repo_config = nullptr) noexcept;
 
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto CreateAction(
         ArtifactDigest const& root_digest,
         std::vector<std::string> const& command,
@@ -49,8 +48,7 @@ class LocalApi final : public IExecutionApi {
         std::vector<std::string> const& output_dirs,
         std::map<std::string, std::string> const& env_vars,
         std::map<std::string, std::string> const& properties,
-        bool force_legacy = false) const noexcept
-        -> IExecutionAction::Ptr final;
+        bool force_legacy) const noexcept -> IExecutionAction::Ptr final;
 
     /// \brief Create a new action (>=RBEv2.1).
     /// \param[in] root_digest  Digest of the build root.
@@ -72,20 +70,16 @@ class LocalApi final : public IExecutionApi {
         std::map<std::string, std::string> const& properties) const noexcept
         -> IExecutionAction::Ptr;
 
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto RetrieveToPaths(
         std::vector<Artifact::ObjectInfo> const& artifacts_info,
         std::vector<std::filesystem::path> const& output_paths,
-        IExecutionApi const* /*alternative*/ = nullptr) const noexcept
-        -> bool final;
+        IExecutionApi const* /*alternative*/) const noexcept -> bool final;
 
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto RetrieveToFds(
         std::vector<Artifact::ObjectInfo> const& artifacts_info,
         std::vector<int> const& fds,
         bool raw_tree,
-        IExecutionApi const* /*alternative*/ = nullptr) const noexcept
-        -> bool final;
+        IExecutionApi const* /*alternative*/) const noexcept -> bool final;
 
     [[nodiscard]] auto RetrieveToCas(
         std::vector<Artifact::ObjectInfo> const& artifacts_info,
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
index 00bb2e75..11cb7659 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
@@ -151,7 +151,6 @@ BazelApi::BazelApi(BazelApi&& other) noexcept = default;
 // implement destructor in cpp, where all members are complete types
 BazelApi::~BazelApi() = default;
 
-// NOLINTNEXTLINE(google-default-arguments)
 auto BazelApi::CreateAction(
     ArtifactDigest const& root_digest,
     std::vector<std::string> const& command,
@@ -191,7 +190,6 @@ auto BazelApi::CreateAction(
                                                         best_effort}};
 }
 
-// NOLINTNEXTLINE(google-default-arguments)
 [[nodiscard]] auto BazelApi::RetrieveToPaths(
     std::vector<Artifact::ObjectInfo> const& artifacts_info,
     std::vector<std::filesystem::path> const& output_paths,
@@ -221,7 +219,8 @@ auto BazelApi::CreateAction(
                 auto const result = reader.RecursivelyReadTreeLeafs(
                     info.digest, output_paths[i]);
                 if (not result or
-                    not RetrieveToPaths(result->infos, result->paths)) {
+                    not RetrieveToPaths(
+                        result->infos, result->paths, alternative)) {
                     return false;
                 }
             }
@@ -266,7 +265,6 @@ auto BazelApi::CreateAction(
     return true;
 }
 
-// NOLINTNEXTLINE(google-default-arguments)
 [[nodiscard]] auto BazelApi::RetrieveToFds(
     std::vector<Artifact::ObjectInfo> const& artifacts_info,
     std::vector<int> const& fds,
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.hpp b/src/buildtool/execution_api/remote/bazel/bazel_api.hpp
index 99e43578..344ae569 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_api.hpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_api.hpp
@@ -58,7 +58,6 @@ class BazelApi final : public IExecutionApi {
     auto operator=(BazelApi&&) -> BazelApi& = delete;
     ~BazelApi() final;
 
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto CreateAction(
         ArtifactDigest const& root_digest,
         std::vector<std::string> const& command,
@@ -67,23 +66,18 @@ class BazelApi final : public IExecutionApi {
         std::vector<std::string> const& output_dirs,
         std::map<std::string, std::string> const& env_vars,
         std::map<std::string, std::string> const& properties,
-        bool force_legacy = false) const noexcept
-        -> IExecutionAction::Ptr final;
+        bool force_legacy) const noexcept -> IExecutionAction::Ptr final;
 
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto RetrieveToPaths(
         std::vector<Artifact::ObjectInfo> const& artifacts_info,
         std::vector<std::filesystem::path> const& output_paths,
-        IExecutionApi const* alternative = nullptr) const noexcept
-        -> bool final;
+        IExecutionApi const* alternative) const noexcept -> bool final;
 
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto RetrieveToFds(
         std::vector<Artifact::ObjectInfo> const& artifacts_info,
         std::vector<int> const& fds,
         bool raw_tree,
-        IExecutionApi const* alternative = nullptr) const noexcept
-        -> bool final;
+        IExecutionApi const* alternative) const noexcept -> bool final;
 
     [[nodiscard]] auto ParallelRetrieveToCas(
         std::vector<Artifact::ObjectInfo> const& artifacts_info,
diff --git a/src/buildtool/execution_api/serve/mr_local_api.cpp b/src/buildtool/execution_api/serve/mr_local_api.cpp
index 9c58d73c..06b75c14 100644
--- a/src/buildtool/execution_api/serve/mr_local_api.cpp
+++ b/src/buildtool/execution_api/serve/mr_local_api.cpp
@@ -34,7 +34,6 @@ MRLocalApi::MRLocalApi(
       native_local_api_{native_local_api},
       compat_local_api_{compatible_local_api} {}
 
-// NOLINTNEXTLINE(google-default-arguments)
 auto MRLocalApi::RetrieveToPaths(
     std::vector<Artifact::ObjectInfo> const& artifacts_info,
     std::vector<std::filesystem::path> const& output_paths,
@@ -94,7 +93,6 @@ auto MRLocalApi::RetrieveToCas(
     return compat_local_api_->RetrieveToCas(*compat_artifacts, api);
 }
 
-// NOLINTNEXTLINE(google-default-arguments)
 auto MRLocalApi::Upload(std::unordered_set<ArtifactBlob>&& blobs,
                         bool skip_find_missing) const noexcept -> bool {
     // in native mode, dispatch to native local api
diff --git a/src/buildtool/execution_api/serve/mr_local_api.hpp b/src/buildtool/execution_api/serve/mr_local_api.hpp
index 738f52fe..237c1267 100644
--- a/src/buildtool/execution_api/serve/mr_local_api.hpp
+++ b/src/buildtool/execution_api/serve/mr_local_api.hpp
@@ -65,20 +65,16 @@ class MRLocalApi final : public IExecutionApi {
     /// Handles both native and compatible artifacts. Dispatches to appropriate
     /// local api instance based on digest hash type. Alternative api is never
     /// used.
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto RetrieveToPaths(
         std::vector<Artifact::ObjectInfo> const& artifacts_info,
         std::vector<std::filesystem::path> const& output_paths,
-        IExecutionApi const* /*alternative*/ = nullptr) const noexcept
-        -> bool final;
+        IExecutionApi const* /*alternative*/) const noexcept -> bool final;
 
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto RetrieveToFds(
         std::vector<Artifact::ObjectInfo> const& /*artifacts_info*/,
         std::vector<int> const& /*fds*/,
         bool /*raw_tree*/,
-        IExecutionApi const* /*alternative*/ = nullptr) const noexcept
-        -> bool final {
+        IExecutionApi const* /*alternative*/) const noexcept -> bool final {
         // Retrieval to file descriptors not supported
         return false;
     }
@@ -104,9 +100,8 @@ class MRLocalApi final : public IExecutionApi {
     /// the blobs to the appropriate local api instance based on used protocol.
     /// \note Caller is responsible for passing vectors with artifacts of the
     /// same digest type.
-    // NOLINTNEXTLINE(google-default-arguments)
     [[nodiscard]] auto Upload(std::unordered_set<ArtifactBlob>&& blobs,
-                              bool skip_find_missing = false) const noexcept
+                              bool skip_find_missing) const noexcept
         -> bool final;
 
     [[nodiscard]] auto UploadTree(
diff --git a/test/buildtool/execution_api/local/local_execution.test.cpp b/test/buildtool/execution_api/local/local_execution.test.cpp
index e1fbec99..d4e4e354 100644
--- a/test/buildtool/execution_api/local/local_execution.test.cpp
+++ b/test/buildtool/execution_api/local/local_execution.test.cpp
@@ -50,6 +50,8 @@
 
 namespace {
 
+constexpr bool kLegacyApi = false;  // do not force legacy api logic
+
 [[nodiscard]] auto GetTestDir() -> std::filesystem::path {
     auto* tmp_dir = std::getenv("TEST_TMPDIR");
     if (tmp_dir != nullptr) {
@@ -95,8 +97,8 @@ TEST_CASE("LocalExecution: No input, no output", "[execution_api]") {
 
     std::string test_content("test");
     std::vector<std::string> const cmdline = {"echo", "-n", test_content};
-    auto action =
-        api.CreateAction(*api.UploadTree({}), cmdline, "", {}, {}, {}, {});
+    auto action = api.CreateAction(
+        *api.UploadTree({}), cmdline, "", {}, {}, {}, {}, kLegacyApi);
     REQUIRE(action);
 
     SECTION("Cache execution result in action cache") {
@@ -155,7 +157,8 @@ TEST_CASE("LocalExecution: No input, no output, env variables used",
                                    {},
                                    {},
                                    {{"MYCONTENT", test_content}},
-                                   {});
+                                   {},
+                                   kLegacyApi);
     REQUIRE(action);
 
     SECTION("Cache execution result in action cache") {
@@ -215,8 +218,14 @@ TEST_CASE("LocalExecution: No input, create output", "[execution_api]") {
         "-c",
         "set -e\necho -n " + test_content + " > " + output_path};
 
-    auto action = api.CreateAction(
-        *api.UploadTree({}), cmdline, "", {output_path}, {}, {}, {});
+    auto action = api.CreateAction(*api.UploadTree({}),
+                                   cmdline,
+                                   "",
+                                   {output_path},
+                                   {},
+                                   {},
+                                   {},
+                                   kLegacyApi);
     REQUIRE(action);
 
     SECTION("Cache execution result in action cache") {
@@ -296,7 +305,8 @@ TEST_CASE("LocalExecution: One input copied to output", "[execution_api]") {
                          {output_path},
                          {},
                          {},
-                         {});
+                         {},
+                         kLegacyApi);
     REQUIRE(action);
 
     SECTION("Cache execution result in action cache") {
@@ -358,8 +368,8 @@ TEST_CASE("LocalExecution: Cache failed action's result", "[execution_api]") {
     std::vector<std::string> const cmdline = {
         "sh", "-c", fmt::format("[ -f '{}' ]", flag.string())};
 
-    auto action =
-        api.CreateAction(*api.UploadTree({}), cmdline, "", {}, {}, {}, {});
+    auto action = api.CreateAction(
+        *api.UploadTree({}), cmdline, "", {}, {}, {}, {}, kLegacyApi);
     REQUIRE(action);
 
     action->SetCacheFlag(IExecutionAction::CacheFlag::CacheOutput);