From 064c415d275433c332b7a38eb99df5d46aaa9f9e Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 24 Oct 2014 10:47:36 -0400 Subject: [PATCH 1/3] test: add test for PARENT_SCOPE behavior Test code courtesy of Alex Merry . --- Tests/RunCMake/set/ParentPulling-stderr.txt | 3 +++ Tests/RunCMake/set/ParentPulling.cmake | 13 +++++++++++++ Tests/RunCMake/set/RunCMakeTest.cmake | 1 + 3 files changed, 17 insertions(+) create mode 100644 Tests/RunCMake/set/ParentPulling-stderr.txt create mode 100644 Tests/RunCMake/set/ParentPulling.cmake diff --git a/Tests/RunCMake/set/ParentPulling-stderr.txt b/Tests/RunCMake/set/ParentPulling-stderr.txt new file mode 100644 index 000000000..768549bfa --- /dev/null +++ b/Tests/RunCMake/set/ParentPulling-stderr.txt @@ -0,0 +1,3 @@ +^before PARENT_SCOPE blah=value2 +after PARENT_SCOPE blah=value2 +in parent scope, blah=value2$ diff --git a/Tests/RunCMake/set/ParentPulling.cmake b/Tests/RunCMake/set/ParentPulling.cmake new file mode 100644 index 000000000..2614533f4 --- /dev/null +++ b/Tests/RunCMake/set/ParentPulling.cmake @@ -0,0 +1,13 @@ +cmake_minimum_required(VERSION 3.0) +project(Minimal NONE) + +function(test_set) + set(blah "value2") + message("before PARENT_SCOPE blah=${blah}") + set(blah ${blah} PARENT_SCOPE) + message("after PARENT_SCOPE blah=${blah}") +endfunction() + +set(blah value1) +test_set() +message("in parent scope, blah=${blah}") diff --git a/Tests/RunCMake/set/RunCMakeTest.cmake b/Tests/RunCMake/set/RunCMakeTest.cmake index 5d036e3b8..9caf53b10 100644 --- a/Tests/RunCMake/set/RunCMakeTest.cmake +++ b/Tests/RunCMake/set/RunCMakeTest.cmake @@ -1,3 +1,4 @@ include(RunCMake) run_cmake(PARENT_SCOPE) +run_cmake(ParentPulling) From e0c0b1ace50f77f2a76dcc7020e3a4251bc6bf96 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 24 Oct 2014 12:37:29 -0400 Subject: [PATCH 2/3] test: add a test for PARENT_SCOPE with multiple scopes See the comment in the test for what is being tested here. --- .../set/ParentPullingRecursive-stderr.txt | 144 ++++++++++++++++++ .../RunCMake/set/ParentPullingRecursive.cmake | 104 +++++++++++++ Tests/RunCMake/set/RunCMakeTest.cmake | 1 + 3 files changed, 249 insertions(+) create mode 100644 Tests/RunCMake/set/ParentPullingRecursive-stderr.txt create mode 100644 Tests/RunCMake/set/ParentPullingRecursive.cmake diff --git a/Tests/RunCMake/set/ParentPullingRecursive-stderr.txt b/Tests/RunCMake/set/ParentPullingRecursive-stderr.txt new file mode 100644 index 000000000..f3260aea9 --- /dev/null +++ b/Tests/RunCMake/set/ParentPullingRecursive-stderr.txt @@ -0,0 +1,144 @@ +---------- +variable values at top before calls: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->top<-- +top_explicit_inner_unset: +top_explicit_inner_tounset: -->top<-- +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_tounset: -->top<-- +outer_implicit_inner_set: +outer_implicit_inner_unset: +outer_explicit_inner_set: +outer_explicit_inner_unset: +outer_explicit_inner_tounset: +---------- +---------- +variable values at outer start: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->top<-- +top_explicit_inner_unset: +top_explicit_inner_tounset: -->top<-- +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_tounset: -->top<-- +outer_implicit_inner_set: +outer_implicit_inner_unset: +outer_explicit_inner_set: +outer_explicit_inner_unset: +outer_explicit_inner_tounset: +---------- +---------- +variable values at outer before inner: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->top<-- +top_explicit_inner_unset: +top_explicit_inner_tounset: -->top<-- +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_tounset: -->top<-- +outer_implicit_inner_set: -->outer<-- +outer_implicit_inner_unset: +outer_explicit_inner_set: -->outer<-- +outer_explicit_inner_unset: +outer_explicit_inner_tounset: -->outer<-- +---------- +---------- +variable values at inner start: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->top<-- +top_explicit_inner_unset: +top_explicit_inner_tounset: -->top<-- +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_tounset: -->top<-- +outer_implicit_inner_set: -->outer<-- +outer_implicit_inner_unset: +outer_explicit_inner_set: -->outer<-- +outer_explicit_inner_unset: +outer_explicit_inner_tounset: -->outer<-- +---------- +---------- +variable values at inner end: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->top<-- +top_explicit_inner_unset: +top_explicit_inner_tounset: -->top<-- +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_tounset: -->top<-- +outer_implicit_inner_set: -->outer<-- +outer_implicit_inner_unset: +outer_explicit_inner_set: -->outer<-- +outer_explicit_inner_unset: +outer_explicit_inner_tounset: -->outer<-- +---------- +---------- +variable values at outer after inner: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->inner<-- +top_explicit_inner_unset: -->inner<-- +top_explicit_inner_tounset: +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_tounset: -->top<-- +outer_implicit_inner_set: -->outer<-- +outer_implicit_inner_unset: +outer_explicit_inner_set: -->inner<-- +outer_explicit_inner_unset: -->inner<-- +outer_explicit_inner_tounset: +---------- +---------- +variable values at outer end: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->inner<-- +top_explicit_inner_unset: -->inner<-- +top_explicit_inner_tounset: +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_set: -->top<-- +top_explicit_outer_unset: +top_explicit_outer_tounset: -->top<-- +outer_implicit_inner_set: -->outer<-- +outer_implicit_inner_unset: +outer_explicit_inner_set: -->inner<-- +outer_explicit_inner_unset: -->inner<-- +outer_explicit_inner_tounset: +---------- +---------- +variable values at top after calls: +top_implicit_inner_set: -->top<-- +top_implicit_inner_unset: +top_explicit_inner_set: -->outer<-- +top_explicit_inner_unset: -->outer<-- +top_explicit_inner_tounset: +top_implicit_outer_set: -->top<-- +top_explicit_outer_unset: -->outer<-- +top_explicit_outer_set: -->outer<-- +top_explicit_outer_unset: -->outer<-- +top_explicit_outer_tounset: +outer_implicit_inner_set: +outer_implicit_inner_unset: +outer_explicit_inner_set: +outer_explicit_inner_unset: +outer_explicit_inner_tounset: +---------- diff --git a/Tests/RunCMake/set/ParentPullingRecursive.cmake b/Tests/RunCMake/set/ParentPullingRecursive.cmake new file mode 100644 index 000000000..a3e29f5b8 --- /dev/null +++ b/Tests/RunCMake/set/ParentPullingRecursive.cmake @@ -0,0 +1,104 @@ +cmake_minimum_required(VERSION 3.0) +project(Minimal NONE) + +function(report where) + message("----------") + message("variable values at ${where}:") + foreach(var IN ITEMS + top_implicit_inner_set top_implicit_inner_unset + top_explicit_inner_set top_explicit_inner_unset top_explicit_inner_tounset + top_implicit_outer_set top_explicit_outer_unset + top_explicit_outer_set top_explicit_outer_unset top_explicit_outer_tounset + + outer_implicit_inner_set outer_implicit_inner_unset + outer_explicit_inner_set outer_explicit_inner_unset outer_explicit_inner_tounset) + if(DEFINED ${var}) + message("${var}: -->${${var}}<--") + else() + message("${var}: ") + endif() + endforeach() + message("----------") +endfunction() + +macro(set_values upscope downscope value) + # Pull the value in implicitly. + set(dummy ${${upscope}_implicit_${downscope}_set}) + set(dummy ${${upscope}_implicit_${downscope}_unset}) + # Pull it down explicitly. + set(${upscope}_explicit_${downscope}_set "${value}" PARENT_SCOPE) + set(${upscope}_explicit_${downscope}_unset "${value}" PARENT_SCOPE) + set(${upscope}_explicit_${downscope}_tounset PARENT_SCOPE) +endmacro() + +function(inner) + report("inner start") + + set_values(top inner inner) + set_values(outer inner inner) + + report("inner end") +endfunction() + +function(outer) + report("outer start") + + set_values(top outer outer) + + # Set values for inner to manipulate. + set(outer_implicit_inner_set outer) + set(outer_implicit_inner_unset) + set(outer_explicit_inner_set outer) + set(outer_explicit_inner_unset) + set(outer_explicit_inner_tounset outer) + + report("outer before inner") + + inner() + + report("outer after inner") + + # Do what inner does so that we can test the values that inner should have + # pulled through to here. + set_values(top inner outer) + + report("outer end") +endfunction() + +# variable name is: +# +# ___ +# +# where the value is the name of the scope it was set in. The scopes available +# are "top", "outer", and "inner". The pull type may either be "implicit" or +# "explicit" based on whether the pull is due to a variable dereference or a +# PARENT_SCOPE setting. The settype is "set" where both scopes set a value, +# "unset" where upscope unsets it and downscope sets it, and "tounset" where +# upscope sets it and downscope unsets it. +# +# We test the following combinations: +# +# - outer overriding top's values; +# - inner overriding top's values; +# - inner overriding outer's values; and +# - outer overriding inner's values in top after inner has run. + +# Set values for inner to manipulate. +set(top_implicit_inner_set top) +set(top_implicit_inner_unset) +set(top_explicit_inner_set top) +set(top_explicit_inner_unset) +set(top_explicit_inner_tounset top) + +# Set values for outer to manipulate. +set(top_implicit_outer_set top) +set(top_implicit_outer_unset) +set(top_explicit_outer_set top) +set(top_explicit_outer_unset) +set(top_explicit_outer_tounset top) + +report("top before calls") + +outer() + +report("top after calls") diff --git a/Tests/RunCMake/set/RunCMakeTest.cmake b/Tests/RunCMake/set/RunCMakeTest.cmake index 9caf53b10..0b96b28d4 100644 --- a/Tests/RunCMake/set/RunCMakeTest.cmake +++ b/Tests/RunCMake/set/RunCMakeTest.cmake @@ -2,3 +2,4 @@ include(RunCMake) run_cmake(PARENT_SCOPE) run_cmake(ParentPulling) +run_cmake(ParentPullingRecursive) From 5f414cefb6524d26329484b296004e3c2d97ec4f Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 24 Oct 2014 12:58:12 -0400 Subject: [PATCH 3/3] Revert "cmDefinitions: Don't store parent lookups" This reverts commit 5abfde6cb8a1ae0b2825797eab6c2e9842eb7c49. The behaviors associated with implicit pulldown on variable lookup seriously conflict with the optimizations made in these commits. Basically, since values were copied upon variable lookup, not just on PARENT_SCOPE, coupled with PARENT_SCOPE's behavior based on whether the variable is in the current scope or not causes serious problems with not storing a value for every variable at every scope. The commit changed behavior of the following example, among other cases: function(test_set) set(blah "value2") message("before PARENT_SCOPE blah=${blah}") set(blah ${blah} PARENT_SCOPE) message("after PARENT_SCOPE blah=${blah}") endfunction() set(blah value1) test_set() message("in parent scope, blah=${blah}") Reported-by: Alex Merry Reported-by: Ben Cooksley --- Source/cmDefinitions.cxx | 22 +++++----------------- Source/cmDefinitions.h | 10 ++++------ Source/cmMakefile.cxx | 2 +- 3 files changed, 10 insertions(+), 24 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 98becf80d..babf1c4e8 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -28,7 +28,7 @@ void cmDefinitions::Reset(cmDefinitions* parent) //---------------------------------------------------------------------------- cmDefinitions::Def const& -cmDefinitions::GetInternal(const std::string& key) const +cmDefinitions::GetInternal(const std::string& key) { MapType::const_iterator i = this->Map.find(key); if(i != this->Map.end()) @@ -37,8 +37,9 @@ cmDefinitions::GetInternal(const std::string& key) const } else if(cmDefinitions* up = this->Up) { - // Query the parent scope. - return up->GetInternal(key); + // Query the parent scope and store the result locally. + Def def = up->GetInternal(key); + return this->Map.insert(MapType::value_type(key, def)).first->second; } return this->NoDef; } @@ -70,25 +71,12 @@ cmDefinitions::SetInternal(const std::string& key, Def const& def) } //---------------------------------------------------------------------------- -const char* cmDefinitions::Get(const std::string& key) const +const char* cmDefinitions::Get(const std::string& key) { Def const& def = this->GetInternal(key); return def.Exists? def.c_str() : 0; } -//---------------------------------------------------------------------------- -void cmDefinitions::Pull(const std::string& key) -{ - if (this->Up) - { - Def const& def = this->Up->GetInternal(key); - if (def.Exists) - { - this->SetInternal(key, def); - } - } -} - //---------------------------------------------------------------------------- const char* cmDefinitions::Set(const std::string& key, const char* value) { diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index ebe6fa5f3..d615fb0e6 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -33,11 +33,9 @@ public: /** Returns the parent scope, if any. */ cmDefinitions* GetParent() const { return this->Up; } - /** Get the value associated with a key; null if none. */ - const char* Get(const std::string& key) const; - - /** Pull a variable from the parent. */ - void Pull(const std::string& key); + /** Get the value associated with a key; null if none. + Store the result locally if it came from a parent. */ + const char* Get(const std::string& key); /** Set (or unset if null) a value associated with a key. */ const char* Set(const std::string& key, const char* value); @@ -75,7 +73,7 @@ private: MapType Map; // Internal query and update methods. - Def const& GetInternal(const std::string& key) const; + Def const& GetInternal(const std::string& key); Def const& SetInternal(const std::string& key, Def const& def); // Implementation of Closure() method. diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 412c998ac..630957fe7 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -4422,7 +4422,7 @@ void cmMakefile::RaiseScope(const std::string& var, const char *varDef) if(cmDefinitions* up = cur.GetParent()) { // First localize the definition in the current scope. - cur.Pull(var); + cur.Get(var); // Now update the definition in the parent scope. up->Set(var, varDef);