GN: Don't define argument overrides globally

tools/gn/args.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "tools/gn/args.h"
 
 #include "base/sys_info.h"
 #include "build/build_config.h"
 #include "tools/gn/variables.h"
 
@@ skipping 71 matching lines @@
 
 }  // namespace
 
 Args::Args() {
 }
 
 Args::Args(const Args& other)
     : overrides_(other.overrides_),
       all_overrides_(other.all_overrides_),
       declared_arguments_per_toolchain_(
-          other.declared_arguments_per_toolchain_) {
+          other.declared_arguments_per_toolchain_),
+      toolchain_overrides_(other.toolchain_overrides_) {
 }
 
 Args::~Args() {
 }
 
 void Args::AddArgOverride(const char* name, const Value& value) {
   base::AutoLock lock(lock_);
 
   overrides_[base::StringPiece(name)] = value;
   all_overrides_[base::StringPiece(name)] = value;
@@ skipping 21 matching lines @@
 Scope::KeyValueMap Args::GetAllOverrides() const {
   base::AutoLock lock(lock_);
   return all_overrides_;
 }
 
 void Args::SetupRootScope(Scope* dest,
                           const Scope::KeyValueMap& toolchain_overrides) const {
   base::AutoLock lock(lock_);
 
   SetSystemVarsLocked(dest);
+
+  // Apply overrides for already declared args.
+  // (i.e. the system vars we set above)
   ApplyOverridesLocked(overrides_, dest);
   ApplyOverridesLocked(toolchain_overrides, dest);
+
+  OverridesForToolchainLocked(dest) = toolchain_overrides;
+
   SaveOverrideRecordLocked(toolchain_overrides);
 }
 
 bool Args::DeclareArgs(const Scope::KeyValueMap& args,
                        Scope* scope_to_set,
                        Err* err) const {
   base::AutoLock lock(lock_);
 
   Scope::KeyValueMap& declared_arguments(
       DeclaredArgumentsForToolchainLocked(scope_to_set));
+
+  const Scope::KeyValueMap& toolchain_overrides(
+      OverridesForToolchainLocked(scope_to_set));
+
   for (const auto& arg : args) {
     // Verify that the value hasn't already been declared. We want each value
     // to be declared only once.
     //
     // The tricky part is that a buildfile can be interpreted multiple times
     // when used from different toolchains, so we can't just check that we've
     // seen it before. Instead, we check that the location matches.
     Scope::KeyValueMap::iterator previously_declared =
         declared_arguments.find(arg.first);
     if (previously_declared != declared_arguments.end()) {
@@ skipping 10 matching lines @@
         err->AppendSubErr(Err(previously_declared->second.origin(),
                               "Previous declaration.",
                               "See also \"gn help buildargs\" for more on how "
                               "build arguments work."));
         return false;
       }
     } else {
       declared_arguments.insert(arg);
     }
 
-    // Only set on the current scope to the new value if it hasn't been already
-    // set. Mark the variable used so the build script can override it in
-    // certain cases without getting unused value errors.
-    if (!scope_to_set->GetValue(arg.first)) {
+    // Check whether this argument has been overridden on the toolchain level
+    // and use the override instead.
+    Scope::KeyValueMap::const_iterator toolchain_override =
+        toolchain_overrides.find(arg.first);
+    if (toolchain_override != toolchain_overrides.end()) {
+      scope_to_set->SetValue(toolchain_override->first,
+                             toolchain_override->second,
+                             toolchain_override->second.origin());
+      continue;
+    }
+
+    // Check whether this argument has been overridden and use the override
+    // instead.
+    Scope::KeyValueMap::const_iterator override = overrides_.find(arg.first);
+    if (override != overrides_.end()) {
+      scope_to_set->SetValue(override->first, override->second,
+                             override->second.origin());

[brettw, 2016/07/11 19:42:30]

I feel like these the toolchain overrides and regular overrides cases should be
symmetrical since they're different cases of the same thing. So can you either
delete the "continue" above and put the rest of this code in an "else" block, or
add a "continue" here and remove the "else" block for the fall-through case
below?

+    } else {
+      // Mark the variable used so the build script can override it in
+      // certain cases without getting unused value errors.
       scope_to_set->SetValue(arg.first, arg.second, arg.second.origin());
       scope_to_set->MarkUsed(arg.first);
     }
   }
 
   return true;
 }
 
 bool Args::VerifyAllOverridesUsed(Err* err) const {
   base::AutoLock lock(lock_);
@@ skipping 89 matching lines @@
   dest->MarkUsed(variables::kCurrentCpu);
   dest->MarkUsed(variables::kTargetCpu);
   dest->MarkUsed(variables::kHostOs);
   dest->MarkUsed(variables::kCurrentOs);
   dest->MarkUsed(variables::kTargetOs);
 }
 
 void Args::ApplyOverridesLocked(const Scope::KeyValueMap& values,
                                 Scope* scope) const {
   lock_.AssertAcquired();
-  for (const auto& val : values)
+
+  const Scope::KeyValueMap& declared_arguments(
+      DeclaredArgumentsForToolchainLocked(scope));
+
+  // Only set a value if it has been declared.
+  for (const auto& val : values) {
+    Scope::KeyValueMap::const_iterator declared =
+        declared_arguments.find(val.first);
+
+    if (declared == declared_arguments.end())
+      continue;
+
     scope->SetValue(val.first, val.second, val.second.origin());
+  }
 }
 
 void Args::SaveOverrideRecordLocked(const Scope::KeyValueMap& values) const {
   lock_.AssertAcquired();
   for (const auto& val : values)
     all_overrides_[val.first] = val.second;
 }
 
 Scope::KeyValueMap& Args::DeclaredArgumentsForToolchainLocked(
     Scope* scope) const {
   lock_.AssertAcquired();
   return declared_arguments_per_toolchain_[scope->settings()];
 }
+
+Scope::KeyValueMap& Args::OverridesForToolchainLocked(
+    Scope* scope) const {
+  lock_.AssertAcquired();
+  return toolchain_overrides_[scope->settings()];
+}