From b43c162e99c7302ae81c746bc5aab1d9a64787b3 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 25 Apr 2015 16:03:04 +0200 Subject: [PATCH 01/16] cmMakefile: Use the Internal class to enclose the VarStack. Put knowledge of the implementation details in one place. --- Source/cmMakefile.cxx | 141 ++++++++++++++++++++++++++++-------------- 1 file changed, 95 insertions(+), 46 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 0935383e9..d71b81517 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -50,6 +50,84 @@ public: std::stack > VarInitStack; std::stack > VarUsageStack; bool IsSourceFileTryCompile; + + void PushDefinitions() + { + cmDefinitions* parent = 0; + if (!this->VarStack.empty()) + { + parent = &this->VarStack.top(); + } + this->VarStack.push(cmDefinitions(parent)); + } + + void InitializeDefinitions(cmMakefile* parent) + { + this->VarStack.top() = parent->Internal->VarStack.top().Closure(); + } + + const char* GetDefinition(std::string const& name) + { + return this->VarStack.top().Get(name); + } + + void SetDefinition(std::string const& name, std::string const& value) + { + this->VarStack.top().Set(name, value.c_str()); + } + + void RemoveDefinition(std::string const& name) + { + this->VarStack.top().Set(name, 0); + } + + std::set LocalKeys() const + { + return this->VarStack.top().LocalKeys(); + } + + std::set ClosureKeys() const + { + return this->VarStack.top().ClosureKeys(); + } + + void PopDefinitions() + { + this->VarStack.pop(); + } + + bool RaiseScope(std::string const& var, const char* varDef, cmMakefile* mf) + { + cmDefinitions& cur = this->VarStack.top(); + if(cmDefinitions* up = cur.GetParent()) + { + // First localize the definition in the current scope. + cur.Get(var); + + // Now update the definition in the parent scope. + up->Set(var, varDef); + } + else if(cmLocalGenerator* plg = mf->GetLocalGenerator()->GetParent()) + { + // Update the definition in the parent directory top scope. This + // directory's scope was initialized by the closure of the parent + // scope, so we do not need to localize the definition first. + cmMakefile* parent = plg->GetMakefile(); + if (varDef) + { + parent->AddDefinition(var, varDef); + } + else + { + parent->RemoveDefinition(var); + } + } + else + { + return false; + } + return true; + } }; // default is not to be building executables @@ -59,11 +137,9 @@ cmMakefile::cmMakefile(cmLocalGenerator* localGenerator) StateSnapshot(localGenerator->GetGlobalGenerator() ->GetCMakeInstance()->GetState()) { - const cmDefinitions& defs = cmDefinitions(); - const std::set globalKeys = defs.LocalKeys(); - this->Internal->VarStack.push(defs); - this->Internal->VarInitStack.push(globalKeys); - this->Internal->VarUsageStack.push(globalKeys); + this->Internal->PushDefinitions(); + this->Internal->VarInitStack.push(std::set()); + this->Internal->VarUsageStack.push(std::set()); this->Internal->IsSourceFileTryCompile = false; if (this->LocalGenerator->GetParent()) @@ -1523,7 +1599,7 @@ void cmMakefile::InitializeFromParent() cmMakefile *parent = this->LocalGenerator->GetParent()->GetMakefile(); // Initialize definitions with the closure of the parent scope. - this->Internal->VarStack.top() = parent->Internal->VarStack.top().Closure(); + this->Internal->InitializeDefinitions(parent); this->AddDefinition("CMAKE_CURRENT_SOURCE_DIR", this->GetCurrentSourceDirectory()); @@ -1717,7 +1793,7 @@ void cmMakefile::AddDefinition(const std::string& name, const char* value) return; } - this->Internal->VarStack.top().Set(name, value); + this->Internal->SetDefinition(name, value); if (!this->Internal->VarUsageStack.empty() && this->VariableInitialized(name)) { @@ -1787,13 +1863,13 @@ void cmMakefile::AddCacheDefinition(const std::string& name, const char* value, this->GetState()->AddCacheEntry(name, haveVal ? val.c_str() : 0, doc, type); // if there was a definition then remove it - this->Internal->VarStack.top().Set(name, 0); + this->Internal->RemoveDefinition(name); } void cmMakefile::AddDefinition(const std::string& name, bool value) { - this->Internal->VarStack.top().Set(name, value? "ON" : "OFF"); + this->Internal->SetDefinition(name, value ? "ON" : "OFF"); if (!this->Internal->VarUsageStack.empty() && this->VariableInitialized(name)) { @@ -1817,8 +1893,7 @@ void cmMakefile::CheckForUnusedVariables() const { return; } - const cmDefinitions& defs = this->Internal->VarStack.top(); - const std::set& locals = defs.LocalKeys(); + const std::set& locals = this->Internal->LocalKeys(); std::set::const_iterator it = locals.begin(); for (; it != locals.end(); ++it) { @@ -1892,7 +1967,7 @@ void cmMakefile::CheckForUnused(const char* reason, void cmMakefile::RemoveDefinition(const std::string& name) { - this->Internal->VarStack.top().Set(name, 0); + this->Internal->RemoveDefinition(name); if (!this->Internal->VarUsageStack.empty() && this->VariableInitialized(name)) { @@ -2365,7 +2440,7 @@ const char* cmMakefile::GetRequiredDefinition(const std::string& name) const bool cmMakefile::IsDefinitionSet(const std::string& name) const { - const char* def = this->Internal->VarStack.top().Get(name); + const char* def = this->Internal->GetDefinition(name); this->Internal->VarUsageStack.top().insert(name); if(!def) { @@ -2391,7 +2466,7 @@ const char* cmMakefile::GetDefinition(const std::string& name) const { this->Internal->VarUsageStack.top().insert(name); } - const char* def = this->Internal->VarStack.top().Get(name); + const char* def = this->Internal->GetDefinition(name); if(!def) { def = this->GetState()->GetInitializedCacheValue(name); @@ -2431,8 +2506,7 @@ std::vector cmMakefile std::vector res; if ( !cacheonly ) { - std::set definitions = - this->Internal->VarStack.top().ClosureKeys(); + std::set definitions = this->Internal->ClosureKeys(); res.insert(res.end(), definitions.begin(), definitions.end()); } std::vector cacheKeys = @@ -4324,10 +4398,9 @@ std::string cmMakefile::GetListFileStack() const void cmMakefile::PushScope() { - cmDefinitions* parent = &this->Internal->VarStack.top(); + this->Internal->PushDefinitions(); const std::set& init = this->Internal->VarInitStack.top(); const std::set& usage = this->Internal->VarUsageStack.top(); - this->Internal->VarStack.push(cmDefinitions(parent)); this->Internal->VarInitStack.push(init); this->Internal->VarUsageStack.push(usage); @@ -4348,10 +4421,9 @@ void cmMakefile::PopScope() this->PopLoopBlockBarrier(); - cmDefinitions* current = &this->Internal->VarStack.top(); std::set init = this->Internal->VarInitStack.top(); std::set usage = this->Internal->VarUsageStack.top(); - const std::set& locals = current->LocalKeys(); + const std::set& locals = this->Internal->LocalKeys(); // Remove initialization and usage information for variables in the local // scope. std::set::const_iterator it = locals.begin(); @@ -4367,7 +4439,8 @@ void cmMakefile::PopScope() usage.erase(*it); } } - this->Internal->VarStack.pop(); + + this->Internal->PopDefinitions(); this->Internal->VarInitStack.pop(); this->Internal->VarUsageStack.pop(); // Push initialization and usage up to the parent scope. @@ -4382,31 +4455,7 @@ void cmMakefile::RaiseScope(const std::string& var, const char *varDef) return; } - cmDefinitions& cur = this->Internal->VarStack.top(); - if(cmDefinitions* up = cur.GetParent()) - { - // First localize the definition in the current scope. - cur.Get(var); - - // Now update the definition in the parent scope. - up->Set(var, varDef); - } - else if(cmLocalGenerator* plg = this->LocalGenerator->GetParent()) - { - // Update the definition in the parent directory top scope. This - // directory's scope was initialized by the closure of the parent - // scope, so we do not need to localize the definition first. - cmMakefile* parent = plg->GetMakefile(); - if (varDef) - { - parent->AddDefinition(var, varDef); - } - else - { - parent->RemoveDefinition(var); - } - } - else + if (!this->Internal->RaiseScope(var, varDef, this)) { std::ostringstream m; m << "Cannot set \"" << var << "\": current scope has no parent."; From 60200ca5088058c70282500994727f2017276df8 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 25 Apr 2015 16:33:26 +0200 Subject: [PATCH 02/16] cmDefinitions: Add an Erase method. --- Source/cmDefinitions.cxx | 5 +++++ Source/cmDefinitions.h | 2 ++ 2 files changed, 7 insertions(+) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index abb46b333..58500c94c 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -61,6 +61,11 @@ void cmDefinitions::Set(const std::string& key, const char* value) } } +void cmDefinitions::Erase(const std::string& key) +{ + this->Map.erase(key); +} + //---------------------------------------------------------------------------- std::set cmDefinitions::LocalKeys() const { diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 83cd0d9ce..13f885df8 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -40,6 +40,8 @@ public: /** Set (or unset if null) a value associated with a key. */ void Set(const std::string& key, const char* value); + void Erase(const std::string& key); + /** Get the set of all local keys. */ std::set LocalKeys() const; From 5067ae41b03442a7dba9210595e782678835a3ff Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 25 Apr 2015 16:36:48 +0200 Subject: [PATCH 03/16] cmDefinitions: Externalize the Set logic. --- Source/cmDefinitions.cxx | 11 +---------- Source/cmMakefile.cxx | 10 +++++++++- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 58500c94c..d2b37bb4a 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -49,16 +49,7 @@ const char* cmDefinitions::Get(const std::string& key) void cmDefinitions::Set(const std::string& key, const char* value) { Def def(value); - if(this->Up || def.Exists) - { - // In lower scopes we store keys, defined or not. - this->Map[key] = def; - } - else - { - // In the top-most scope we need not store undefined keys. - this->Map.erase(key); - } + this->Map[key] = def; } void cmDefinitions::Erase(const std::string& key) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index d71b81517..875442762 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -78,7 +78,15 @@ public: void RemoveDefinition(std::string const& name) { - this->VarStack.top().Set(name, 0); + if (this->VarStack.size() > 1) + { + // In lower scopes we store keys, defined or not. + this->VarStack.top().Set(name, 0); + } + else + { + this->VarStack.top().Erase(name); + } } std::set LocalKeys() const From 818bf727c1eb4a500decb5856715a964c948242e Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 16:34:13 +0200 Subject: [PATCH 04/16] cmDefinitions: Change LocalKeys to return a vector. This is more efficient and we lose nothing. --- Source/cmDefinitions.cxx | 7 ++++--- Source/cmDefinitions.h | 2 +- Source/cmMakefile.cxx | 10 +++++----- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index d2b37bb4a..448ba9d0e 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -58,16 +58,17 @@ void cmDefinitions::Erase(const std::string& key) } //---------------------------------------------------------------------------- -std::set cmDefinitions::LocalKeys() const +std::vector cmDefinitions::LocalKeys() const { - std::set keys; + std::vector keys; + keys.reserve(this->Map.size()); // Consider local definitions. for(MapType::const_iterator mi = this->Map.begin(); mi != this->Map.end(); ++mi) { if (mi->second.Exists) { - keys.insert(mi->first); + keys.push_back(mi->first); } } return keys; diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 13f885df8..d21ec23f1 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -43,7 +43,7 @@ public: void Erase(const std::string& key); /** Get the set of all local keys. */ - std::set LocalKeys() const; + std::vector LocalKeys() const; /** Compute the closure of all defined keys with values. This flattens the scope. The result has no parent. */ diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 875442762..9209e498d 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -89,7 +89,7 @@ public: } } - std::set LocalKeys() const + std::vector LocalKeys() const { return this->VarStack.top().LocalKeys(); } @@ -1901,8 +1901,8 @@ void cmMakefile::CheckForUnusedVariables() const { return; } - const std::set& locals = this->Internal->LocalKeys(); - std::set::const_iterator it = locals.begin(); + const std::vector& locals = this->Internal->LocalKeys(); + std::vector::const_iterator it = locals.begin(); for (; it != locals.end(); ++it) { this->CheckForUnused("out of scope", *it); @@ -4431,10 +4431,10 @@ void cmMakefile::PopScope() std::set init = this->Internal->VarInitStack.top(); std::set usage = this->Internal->VarUsageStack.top(); - const std::set& locals = this->Internal->LocalKeys(); + const std::vector& locals = this->Internal->LocalKeys(); // Remove initialization and usage information for variables in the local // scope. - std::set::const_iterator it = locals.begin(); + std::vector::const_iterator it = locals.begin(); for (; it != locals.end(); ++it) { init.erase(*it); From 78e1454ea09e9c568578e26971d6cd45b7fa39c7 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 11:38:08 +0200 Subject: [PATCH 05/16] cmDefinitions: Replace ClosureKeys recursion with looping. --- Source/cmDefinitions.cxx | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 448ba9d0e..a6f46e2ea 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -131,22 +131,22 @@ std::set cmDefinitions::ClosureKeys() const void cmDefinitions::ClosureKeys(std::set& defined, std::set& undefined) const { - // Consider local definitions. - for(MapType::const_iterator mi = this->Map.begin(); - mi != this->Map.end(); ++mi) - { - // Use this key if it is not already set or unset. - if(defined.find(mi->first) == defined.end() && - undefined.find(mi->first) == undefined.end()) - { - std::set& m = mi->second.Exists? defined : undefined; - m.insert(mi->first); - } - } + cmDefinitions const* up = this; - // Traverse parents. - if(cmDefinitions const* up = this->Up) + while (up) { - up->ClosureKeys(defined, undefined); + // Consider local definitions. + for(MapType::const_iterator mi = up->Map.begin(); + mi != up->Map.end(); ++mi) + { + // Use this key if it is not already set or unset. + if(defined.find(mi->first) == defined.end() && + undefined.find(mi->first) == undefined.end()) + { + std::set& m = mi->second.Exists? defined : undefined; + m.insert(mi->first); + } + } + up = up->Up; } } From ca9fa77d5d34a993073cd5256d65f88cd2e1a28f Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 15:38:09 +0200 Subject: [PATCH 06/16] cmDefinitions: Inline GetClosureKeys implementation. --- Source/cmDefinitions.cxx | 8 +------- Source/cmDefinitions.h | 4 ---- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index a6f46e2ea..4b0eed459 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -123,14 +123,7 @@ std::set cmDefinitions::ClosureKeys() const { std::set defined; std::set undefined; - this->ClosureKeys(defined, undefined); - return defined; -} -//---------------------------------------------------------------------------- -void cmDefinitions::ClosureKeys(std::set& defined, - std::set& undefined) const -{ cmDefinitions const* up = this; while (up) @@ -149,4 +142,5 @@ void cmDefinitions::ClosureKeys(std::set& defined, } up = up->Up; } + return defined; } diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index d21ec23f1..950970bb4 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -86,10 +86,6 @@ private: cmDefinitions(ClosureTag const&, cmDefinitions const* root); void ClosureImpl(std::set& undefined, cmDefinitions const* defs); - - // Implementation of ClosureKeys() method. - void ClosureKeys(std::set& defined, - std::set& undefined) const; }; #endif From 012a75a00f060d6ca36cc9ffb97439a7ad526395 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 15:36:49 +0200 Subject: [PATCH 07/16] cmDefinitions: Make ClosureKeys API vector-based. Construct the final list directly in a named return value. Use a single set to track bindings already found. Co-Author: Brad King --- Source/cmDefinitions.cxx | 12 +++++------- Source/cmDefinitions.h | 2 +- Source/cmMakefile.cxx | 5 ++--- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 4b0eed459..f2100eb6a 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -119,10 +119,10 @@ void cmDefinitions::ClosureImpl(std::set& undefined, } //---------------------------------------------------------------------------- -std::set cmDefinitions::ClosureKeys() const +std::vector cmDefinitions::ClosureKeys() const { - std::set defined; - std::set undefined; + std::vector defined; + std::set bound; cmDefinitions const* up = this; @@ -133,11 +133,9 @@ std::set cmDefinitions::ClosureKeys() const mi != up->Map.end(); ++mi) { // Use this key if it is not already set or unset. - if(defined.find(mi->first) == defined.end() && - undefined.find(mi->first) == undefined.end()) + if(bound.insert(mi->first).second && mi->second.Exists) { - std::set& m = mi->second.Exists? defined : undefined; - m.insert(mi->first); + defined.push_back(mi->first); } } up = up->Up; diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 950970bb4..4664090bf 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -50,7 +50,7 @@ public: cmDefinitions Closure() const; /** Compute the set of all defined keys. */ - std::set ClosureKeys() const; + std::vector ClosureKeys() const; private: // String with existence boolean. diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 9209e498d..e2a9f61f5 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -94,7 +94,7 @@ public: return this->VarStack.top().LocalKeys(); } - std::set ClosureKeys() const + std::vector ClosureKeys() const { return this->VarStack.top().ClosureKeys(); } @@ -2514,8 +2514,7 @@ std::vector cmMakefile std::vector res; if ( !cacheonly ) { - std::set definitions = this->Internal->ClosureKeys(); - res.insert(res.end(), definitions.begin(), definitions.end()); + res = this->Internal->ClosureKeys(); } std::vector cacheKeys = this->GetState()->GetCacheEntryKeys(); From 24885d4efa17d4232e266ef053899613c32fdeb7 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 15:44:26 +0200 Subject: [PATCH 08/16] cmDefinitions: Replace private constructor with MakeClosure. --- Source/cmDefinitions.cxx | 17 ++++++----------- Source/cmDefinitions.h | 11 +++-------- Source/cmMakefile.cxx | 2 +- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index f2100eb6a..c4d285a73 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -75,21 +75,16 @@ std::vector cmDefinitions::LocalKeys() const } //---------------------------------------------------------------------------- -cmDefinitions cmDefinitions::Closure() const -{ - return cmDefinitions(ClosureTag(), this); -} - -//---------------------------------------------------------------------------- -cmDefinitions::cmDefinitions(ClosureTag const&, cmDefinitions const* root): - Up(0) +cmDefinitions cmDefinitions::MakeClosure() const { std::set undefined; - this->ClosureImpl(undefined, root); + cmDefinitions closure; + closure.MakeClosure(undefined, this); + return closure; } //---------------------------------------------------------------------------- -void cmDefinitions::ClosureImpl(std::set& undefined, +void cmDefinitions::MakeClosure(std::set& undefined, cmDefinitions const* defs) { // Consider local definitions. @@ -114,7 +109,7 @@ void cmDefinitions::ClosureImpl(std::set& undefined, // Traverse parents. if(cmDefinitions const* up = defs->Up) { - this->ClosureImpl(undefined, up); + this->MakeClosure(undefined, up); } } diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 4664090bf..691740271 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -45,13 +45,11 @@ public: /** Get the set of all local keys. */ std::vector LocalKeys() const; - /** Compute the closure of all defined keys with values. - This flattens the scope. The result has no parent. */ - cmDefinitions Closure() const; - /** Compute the set of all defined keys. */ std::vector ClosureKeys() const; + cmDefinitions MakeClosure() const; + private: // String with existence boolean. struct Def: public std::string @@ -81,10 +79,7 @@ private: // Internal query and update methods. Def const& GetInternal(const std::string& key); - // Implementation of Closure() method. - struct ClosureTag {}; - cmDefinitions(ClosureTag const&, cmDefinitions const* root); - void ClosureImpl(std::set& undefined, + void MakeClosure(std::set& undefined, cmDefinitions const* defs); }; diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index e2a9f61f5..dbb355c86 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -63,7 +63,7 @@ public: void InitializeDefinitions(cmMakefile* parent) { - this->VarStack.top() = parent->Internal->VarStack.top().Closure(); + this->VarStack.top() = parent->Internal->VarStack.top().MakeClosure(); } const char* GetDefinition(std::string const& name) From f983d8913b3293f0f328811d243222c6e19c795e Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 15:49:43 +0200 Subject: [PATCH 09/16] cmDefinitions: Replace recursion with loop. --- Source/cmDefinitions.cxx | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index c4d285a73..2ee2618d0 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -87,29 +87,27 @@ cmDefinitions cmDefinitions::MakeClosure() const void cmDefinitions::MakeClosure(std::set& undefined, cmDefinitions const* defs) { - // Consider local definitions. - for(MapType::const_iterator mi = defs->Map.begin(); - mi != defs->Map.end(); ++mi) + while(defs) { - // Use this key if it is not already set or unset. - if(this->Map.find(mi->first) == this->Map.end() && - undefined.find(mi->first) == undefined.end()) + // Consider local definitions. + for(MapType::const_iterator mi = defs->Map.begin(); + mi != defs->Map.end(); ++mi) { - if(mi->second.Exists) + // Use this key if it is not already set or unset. + if(this->Map.find(mi->first) == this->Map.end() && + undefined.find(mi->first) == undefined.end()) { - this->Map.insert(*mi); - } - else - { - undefined.insert(mi->first); + if(mi->second.Exists) + { + this->Map.insert(*mi); + } + else + { + undefined.insert(mi->first); + } } } - } - - // Traverse parents. - if(cmDefinitions const* up = defs->Up) - { - this->MakeClosure(undefined, up); + defs = defs->Up; } } From aaaa65b6a5cdf227282a9d04bf5287413757ff03 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 25 Apr 2015 17:21:51 +0200 Subject: [PATCH 10/16] cmMakefile: Remove stack adaptor for the VarStack. The purpose of the stack is to allow access only to the top of it. Access to items which are not at the top is needed, so cmDefinitions objects get a Parent pointer. The existence of the Parent pointer is a workaround for the inappropriate use of stack in the first place. Remove it now. --- Source/cmMakefile.cxx | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index dbb355c86..8cfb83a57 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -46,7 +46,7 @@ class cmMakefile::Internals { public: - std::stack > VarStack; + std::list VarStack; std::stack > VarInitStack; std::stack > VarUsageStack; bool IsSourceFileTryCompile; @@ -56,24 +56,24 @@ public: cmDefinitions* parent = 0; if (!this->VarStack.empty()) { - parent = &this->VarStack.top(); + parent = &this->VarStack.back(); } - this->VarStack.push(cmDefinitions(parent)); + this->VarStack.push_back(cmDefinitions(parent)); } void InitializeDefinitions(cmMakefile* parent) { - this->VarStack.top() = parent->Internal->VarStack.top().MakeClosure(); + this->VarStack.back() = parent->Internal->VarStack.back().MakeClosure(); } const char* GetDefinition(std::string const& name) { - return this->VarStack.top().Get(name); + return this->VarStack.back().Get(name); } void SetDefinition(std::string const& name, std::string const& value) { - this->VarStack.top().Set(name, value.c_str()); + this->VarStack.back().Set(name, value.c_str()); } void RemoveDefinition(std::string const& name) @@ -81,32 +81,32 @@ public: if (this->VarStack.size() > 1) { // In lower scopes we store keys, defined or not. - this->VarStack.top().Set(name, 0); + this->VarStack.back().Set(name, 0); } else { - this->VarStack.top().Erase(name); + this->VarStack.back().Erase(name); } } std::vector LocalKeys() const { - return this->VarStack.top().LocalKeys(); + return this->VarStack.back().LocalKeys(); } std::vector ClosureKeys() const { - return this->VarStack.top().ClosureKeys(); + return this->VarStack.back().ClosureKeys(); } void PopDefinitions() { - this->VarStack.pop(); + this->VarStack.pop_back(); } bool RaiseScope(std::string const& var, const char* varDef, cmMakefile* mf) { - cmDefinitions& cur = this->VarStack.top(); + cmDefinitions& cur = this->VarStack.back(); if(cmDefinitions* up = cur.GetParent()) { // First localize the definition in the current scope. From d858f36339d61e45913165bc957d645bf1060f54 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 15:54:02 +0200 Subject: [PATCH 11/16] cmDefinitions: Use list of cmDefinitions* to create closure. --- Source/cmDefinitions.cxx | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 2ee2618d0..5fe57c84c 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -11,6 +11,8 @@ ============================================================================*/ #include "cmDefinitions.h" +#include + //---------------------------------------------------------------------------- cmDefinitions::Def cmDefinitions::NoDef; @@ -87,11 +89,18 @@ cmDefinitions cmDefinitions::MakeClosure() const void cmDefinitions::MakeClosure(std::set& undefined, cmDefinitions const* defs) { + std::list ups; while(defs) { + ups.push_back(defs); + defs = defs->Up; + } + for (std::list::const_iterator it = ups.begin(); + it != ups.end(); ++it) + { // Consider local definitions. - for(MapType::const_iterator mi = defs->Map.begin(); - mi != defs->Map.end(); ++mi) + for(MapType::const_iterator mi = (*it)->Map.begin(); + mi != (*it)->Map.end(); ++mi) { // Use this key if it is not already set or unset. if(this->Map.find(mi->first) == this->Map.end() && @@ -107,7 +116,6 @@ void cmDefinitions::MakeClosure(std::set& undefined, } } } - defs = defs->Up; } } From 60becdc65c5f8cfad4b2c6a33e3649d2acbddf39 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 16:00:18 +0200 Subject: [PATCH 12/16] cmDefinitions: Implement MakeClosure in terms of a list of ancestors. --- Source/cmDefinitions.cxx | 25 +++++++++++++------------ Source/cmDefinitions.h | 5 ++++- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 5fe57c84c..873a7b4ec 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -11,8 +11,6 @@ ============================================================================*/ #include "cmDefinitions.h" -#include - //---------------------------------------------------------------------------- cmDefinitions::Def cmDefinitions::NoDef; @@ -81,22 +79,25 @@ cmDefinitions cmDefinitions::MakeClosure() const { std::set undefined; cmDefinitions closure; - closure.MakeClosure(undefined, this); - return closure; -} - -//---------------------------------------------------------------------------- -void cmDefinitions::MakeClosure(std::set& undefined, - cmDefinitions const* defs) -{ + cmDefinitions const* defs = this; std::list ups; while(defs) { ups.push_back(defs); defs = defs->Up; } - for (std::list::const_iterator it = ups.begin(); - it != ups.end(); ++it) + closure.MakeClosure(undefined, ups.begin(), ups.end()); + return closure; +} + +//---------------------------------------------------------------------------- +void +cmDefinitions::MakeClosure(std::set& undefined, + std::list::iterator begin, + std::list::iterator end) +{ + for (std::list::const_iterator it = begin; + it != end; ++it) { // Consider local definitions. for(MapType::const_iterator mi = (*it)->Map.begin(); diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 691740271..40e053198 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -17,6 +17,8 @@ #include "cmsys/hash_map.hxx" #endif +#include + /** \class cmDefinitions * \brief Store a scope of variable definitions for CMake language. * @@ -80,7 +82,8 @@ private: Def const& GetInternal(const std::string& key); void MakeClosure(std::set& undefined, - cmDefinitions const* defs); + std::list::iterator begin, + std::list::iterator end); }; #endif From aa4d1ee80f1ced5b09335cc84bdd373c0875fd80 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 16:13:56 +0200 Subject: [PATCH 13/16] cmDefinitions: Convert MakeClosure into a static method. Accept a range of cmDefinitions*. --- Source/cmDefinitions.cxx | 13 ++++--------- Source/cmDefinitions.h | 4 +++- Source/cmMakefile.cxx | 10 +++++++++- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 873a7b4ec..d1fbe7446 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -75,18 +75,13 @@ std::vector cmDefinitions::LocalKeys() const } //---------------------------------------------------------------------------- -cmDefinitions cmDefinitions::MakeClosure() const +cmDefinitions cmDefinitions::MakeClosure( + std::list::iterator begin, + std::list::iterator end) { std::set undefined; cmDefinitions closure; - cmDefinitions const* defs = this; - std::list ups; - while(defs) - { - ups.push_back(defs); - defs = defs->Up; - } - closure.MakeClosure(undefined, ups.begin(), ups.end()); + closure.MakeClosure(undefined, begin, end); return closure; } diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 40e053198..67b617069 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -50,7 +50,9 @@ public: /** Compute the set of all defined keys. */ std::vector ClosureKeys() const; - cmDefinitions MakeClosure() const; + static cmDefinitions MakeClosure( + std::list::iterator begin, + std::list::iterator end); private: // String with existence boolean. diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 8cfb83a57..879709073 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -63,7 +63,15 @@ public: void InitializeDefinitions(cmMakefile* parent) { - this->VarStack.back() = parent->Internal->VarStack.back().MakeClosure(); + std::list defPtrs; + for (std::list::iterator it = + parent->Internal->VarStack.begin(); + it != parent->Internal->VarStack.end(); ++it) + { + defPtrs.push_back(&*it); + } + this->VarStack.back() = cmDefinitions::MakeClosure(defPtrs.begin(), + defPtrs.end()); } const char* GetDefinition(std::string const& name) From f79cd99d6dcdfcdcd341c5ea90a5f2d9c4d6d3bc Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Wed, 29 Apr 2015 23:48:43 +0200 Subject: [PATCH 14/16] cmDefinitions: Implement MakeClosure in terms of reverse iterators. Iterate directly over the parent content provided by cmMakefile. --- Source/cmDefinitions.cxx | 18 +++++++++--------- Source/cmDefinitions.h | 8 ++++---- Source/cmMakefile.cxx | 12 +++--------- 3 files changed, 16 insertions(+), 22 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index d1fbe7446..718d9ec55 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -76,27 +76,27 @@ std::vector cmDefinitions::LocalKeys() const //---------------------------------------------------------------------------- cmDefinitions cmDefinitions::MakeClosure( - std::list::iterator begin, - std::list::iterator end) + std::list::const_reverse_iterator rbegin, + std::list::const_reverse_iterator rend) { std::set undefined; cmDefinitions closure; - closure.MakeClosure(undefined, begin, end); + closure.MakeClosure(undefined, rbegin, rend); return closure; } //---------------------------------------------------------------------------- void cmDefinitions::MakeClosure(std::set& undefined, - std::list::iterator begin, - std::list::iterator end) + std::list::const_reverse_iterator rbegin, + std::list::const_reverse_iterator rend) { - for (std::list::const_iterator it = begin; - it != end; ++it) + for (std::list::const_reverse_iterator it = rbegin; + it != rend; ++it) { // Consider local definitions. - for(MapType::const_iterator mi = (*it)->Map.begin(); - mi != (*it)->Map.end(); ++mi) + for(MapType::const_iterator mi = it->Map.begin(); + mi != it->Map.end(); ++mi) { // Use this key if it is not already set or unset. if(this->Map.find(mi->first) == this->Map.end() && diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 67b617069..87f5c3401 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -51,8 +51,8 @@ public: std::vector ClosureKeys() const; static cmDefinitions MakeClosure( - std::list::iterator begin, - std::list::iterator end); + std::list::const_reverse_iterator rbegin, + std::list::const_reverse_iterator rend); private: // String with existence boolean. @@ -84,8 +84,8 @@ private: Def const& GetInternal(const std::string& key); void MakeClosure(std::set& undefined, - std::list::iterator begin, - std::list::iterator end); + std::list::const_reverse_iterator rbegin, + std::list::const_reverse_iterator rend); }; #endif diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 879709073..6451874ac 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -63,15 +63,9 @@ public: void InitializeDefinitions(cmMakefile* parent) { - std::list defPtrs; - for (std::list::iterator it = - parent->Internal->VarStack.begin(); - it != parent->Internal->VarStack.end(); ++it) - { - defPtrs.push_back(&*it); - } - this->VarStack.back() = cmDefinitions::MakeClosure(defPtrs.begin(), - defPtrs.end()); + this->VarStack.back() = + cmDefinitions::MakeClosure(parent->Internal->VarStack.rbegin(), + parent->Internal->VarStack.rend()); } const char* GetDefinition(std::string const& name) From 5ccff6408c93e67d7f8445ce1bf6465b068d6f6b Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 15:38:36 +0200 Subject: [PATCH 15/16] cmDefinitions: Externalize looping for ClosureKeys. --- Source/cmDefinitions.cxx | 23 ++++++++--------------- Source/cmDefinitions.h | 4 ++-- Source/cmMakefile.cxx | 11 ++++++++++- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 718d9ec55..115e30a4f 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -116,26 +116,19 @@ cmDefinitions::MakeClosure(std::set& undefined, } //---------------------------------------------------------------------------- -std::vector cmDefinitions::ClosureKeys() const +std::vector +cmDefinitions::ClosureKeys(std::set& bound) const { std::vector defined; - std::set bound; - - cmDefinitions const* up = this; - - while (up) + defined.reserve(this->Map.size()); + for(MapType::const_iterator mi = this->Map.begin(); + mi != this->Map.end(); ++mi) { - // Consider local definitions. - for(MapType::const_iterator mi = up->Map.begin(); - mi != up->Map.end(); ++mi) + // Use this key if it is not already set or unset. + if(bound.insert(mi->first).second && mi->second.Exists) { - // Use this key if it is not already set or unset. - if(bound.insert(mi->first).second && mi->second.Exists) - { - defined.push_back(mi->first); - } + defined.push_back(mi->first); } - up = up->Up; } return defined; } diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 87f5c3401..4c7f11f60 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -47,8 +47,8 @@ public: /** Get the set of all local keys. */ std::vector LocalKeys() const; - /** Compute the set of all defined keys. */ - std::vector ClosureKeys() const; + std::vector + ClosureKeys(std::set& bound) const; static cmDefinitions MakeClosure( std::list::const_reverse_iterator rbegin, diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 6451874ac..1d3e4965a 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -98,7 +98,16 @@ public: std::vector ClosureKeys() const { - return this->VarStack.back().ClosureKeys(); + std::vector closureKeys; + std::set bound; + for (std::list::const_reverse_iterator it = + this->VarStack.rbegin(); it != this->VarStack.rend(); ++it) + { + std::vector const& localKeys = it->ClosureKeys(bound); + closureKeys.insert(closureKeys.end(), + localKeys.begin(), localKeys.end()); + } + return closureKeys; } void PopDefinitions() From b48ea26a9d3b4384874554fe64edfce8784a9255 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 26 Apr 2015 12:39:53 +0200 Subject: [PATCH 16/16] cmDefinitions: Invert conditional code. Return the simple case first. --- Source/cmDefinitions.cxx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 115e30a4f..61328be60 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -29,13 +29,14 @@ cmDefinitions::GetInternal(const std::string& key) { return i->second; } - if(cmDefinitions* up = this->Up) + cmDefinitions* up = this->Up; + if(!up) { - // 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; } - return this->NoDef; + // 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; } //----------------------------------------------------------------------------