From 6b9e647239b67aff3119efbe2691fefe9c2dc28b Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Thu, 14 May 2015 23:47:40 +0200 Subject: [PATCH 01/18] cmMakefile: Port CurrentListFile clients to GetDefinition. There is no need to store this as a member variable. --- Source/cmExtraQbsGenerator.cxx | 4 +++- Source/cmLocalNinjaGenerator.cxx | 4 ++-- Source/cmMakefile.cxx | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Source/cmExtraQbsGenerator.cxx b/Source/cmExtraQbsGenerator.cxx index 4cc465048..c15f8dae7 100644 --- a/Source/cmExtraQbsGenerator.cxx +++ b/Source/cmExtraQbsGenerator.cxx @@ -182,7 +182,9 @@ void cmExtraQbsGenerator::AppendSources(cmGeneratedFileStream &fout, std::vector genSources; std::vector::const_iterator itr = sources.begin(); fout << "\t\t\tfiles: [\n" - << "\t\t\t\t\"" << t.GetMakefile()->GetCurrentListFile() << "\",\n"; + << "\t\t\t\t\"" + << t.GetMakefile()->GetDefinition("CMAKE_CURRENT_LIST_FILE") + << "\",\n"; for (; itr != sources.end(); ++itr) { if (!(*itr)->GetPropertyAsBool("GENERATED")) diff --git a/Source/cmLocalNinjaGenerator.cxx b/Source/cmLocalNinjaGenerator.cxx index f1f1202a3..bcae486dd 100644 --- a/Source/cmLocalNinjaGenerator.cxx +++ b/Source/cmLocalNinjaGenerator.cxx @@ -305,8 +305,8 @@ void cmLocalNinjaGenerator::WriteProcessedMakefile(std::ostream& os) cmGlobalNinjaGenerator::WriteDivider(os); os << "# Write statements declared in CMakeLists.txt:" << std::endl - << "# " << this->Makefile->GetCurrentListFile() << std::endl - ; + << "# " + << this->Makefile->GetDefinition("CMAKE_CURRENT_LIST_FILE") << std::endl; if(this->IsRootMakefile()) os << "# Which is the root file." << std::endl; cmGlobalNinjaGenerator::WriteDivider(os); diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 3e19cbbdb..0a112cb24 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -609,7 +609,8 @@ bool cmMakefile::ProcessBuildsystemFile(const char* listfile) bool cmMakefile::ReadDependentFile(const char* listfile, bool noPolicyScope) { - this->AddDefinition("CMAKE_PARENT_LIST_FILE", this->GetCurrentListFile()); + this->AddDefinition("CMAKE_PARENT_LIST_FILE", + this->GetDefinition("CMAKE_CURRENT_LIST_FILE")); this->cmCurrentListFile = cmSystemTools::CollapseFullPath(listfile, this->GetCurrentSourceDirectory()); From f3e6a336f29b0675e4eb3852ce7544f3c9342071 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Thu, 14 May 2015 23:49:10 +0200 Subject: [PATCH 02/18] cmMakefile: Remove CurrentListFile member. It is never read externally. The CollapseFullPath removed in this commit is a repeat of a similar call inside ReadListFile. --- Source/cmMakefile.cxx | 7 +------ Source/cmMakefile.h | 10 ---------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 0a112cb24..e69c2bab0 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -601,7 +601,6 @@ void cmMakefile::IncludeScope::EnforceCMP0011() bool cmMakefile::ProcessBuildsystemFile(const char* listfile) { this->AddDefinition("CMAKE_PARENT_LIST_FILE", listfile); - this->cmCurrentListFile = listfile; std::string curSrc = this->GetCurrentSourceDirectory(); return this->ReadListFile(listfile, true, curSrc == this->GetHomeDirectory()); @@ -611,11 +610,7 @@ bool cmMakefile::ReadDependentFile(const char* listfile, bool noPolicyScope) { this->AddDefinition("CMAKE_PARENT_LIST_FILE", this->GetDefinition("CMAKE_CURRENT_LIST_FILE")); - this->cmCurrentListFile = - cmSystemTools::CollapseFullPath(listfile, - this->GetCurrentSourceDirectory()); - return this->ReadListFile(this->cmCurrentListFile.c_str(), - noPolicyScope); + return this->ReadListFile(listfile, noPolicyScope); } //---------------------------------------------------------------------------- diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index 82f271546..2a72cca5d 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -418,14 +418,6 @@ public: void SetCurrentBinaryDirectory(const std::string& dir); const char* GetCurrentBinaryDirectory() const; - /* Get the current CMakeLists.txt file that is being processed. This - * is just used in order to be able to 'branch' from one file to a second - * transparently */ - const char* GetCurrentListFile() const - { - return this->cmCurrentListFile.c_str(); - } - //@} /** @@ -845,8 +837,6 @@ protected: // Check for a an unused variable void CheckForUnused(const char* reason, const std::string& name) const; - std::string cmCurrentListFile; - std::string ProjectName; // project name // libraries, classes, and executables From 2dd5d42f5254abda64009434f0b11a47ca16640e Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Fri, 15 May 2015 00:09:53 +0200 Subject: [PATCH 03/18] cmMakefile: Make the public ReadListFile method take one param. Make the existing method a private overload. All external callers invoke the method with only one argument. --- Source/cmMakefile.cxx | 10 ++++++---- Source/cmMakefile.h | 11 +++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index e69c2bab0..016425e14 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -610,12 +610,14 @@ bool cmMakefile::ReadDependentFile(const char* listfile, bool noPolicyScope) { this->AddDefinition("CMAKE_PARENT_LIST_FILE", this->GetDefinition("CMAKE_CURRENT_LIST_FILE")); - return this->ReadListFile(listfile, noPolicyScope); + return this->ReadListFile(listfile, noPolicyScope, false); +} + +bool cmMakefile::ReadListFile(const char* listfile) +{ + return this->ReadListFile(listfile, true, false); } -//---------------------------------------------------------------------------- -// Parse the given CMakeLists.txt file executing all commands -// bool cmMakefile::ReadListFile(const char* listfile, bool noPolicyScope, bool requireProjectCommand) diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index 2a72cca5d..8c50e8ee0 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -84,12 +84,7 @@ public: */ ~cmMakefile(); - /** - * Read and parse a CMakeLists.txt file. - */ - bool ReadListFile(const char* listfile, - bool noPolicyScope = true, - bool requireProjectCommand = false); + bool ReadListFile(const char* listfile); bool ReadDependentFile(const char* listfile, bool noPolicyScope = true); @@ -902,6 +897,10 @@ private: cmState::Snapshot StateSnapshot; + bool ReadListFile(const char* listfile, + bool noPolicyScope, + bool requireProjectCommand); + bool ReadListFileInternal(const char* filenametoread, bool noPolicyScope, bool requireProjectCommand); From 8ab1cce7047b637f504b4b83cecc104e5cd1adc5 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 16 May 2015 07:30:33 +0200 Subject: [PATCH 04/18] cmMakefile: Rename method to something more appropriate. Allow the name to be used for something more-suitable. --- Source/cmMakefile.cxx | 2 +- Source/cmMakefile.h | 2 +- Source/cmTryRunCommand.cxx | 2 +- Source/cmakemain.cxx | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 016425e14..c1bbd12a3 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -4362,7 +4362,7 @@ void cmMakefile::AddCMakeDependFilesFromUser() } } -std::string cmMakefile::GetListFileStack() const +std::string cmMakefile::FormatListFileStack() const { std::ostringstream tmp; size_t depth = this->ListFileStack.size(); diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index 8c50e8ee0..72d8ec6d4 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -575,7 +575,7 @@ public: { this->ListFiles.push_back(file);} void AddCMakeDependFilesFromUser(); - std::string GetListFileStack() const; + std::string FormatListFileStack() const; /** * Get the current context backtrace. diff --git a/Source/cmTryRunCommand.cxx b/Source/cmTryRunCommand.cxx index 3cd92cb8e..b9ffe5e0f 100644 --- a/Source/cmTryRunCommand.cxx +++ b/Source/cmTryRunCommand.cxx @@ -375,7 +375,7 @@ void cmTryRunCommand::DoNotRunExecutable(const std::string& runArgs, comment += "Run arguments : "; comment += runArgs; comment += "\n"; - comment += " Called from: " + this->Makefile->GetListFileStack(); + comment += " Called from: " + this->Makefile->FormatListFileStack(); cmsys::SystemTools::ReplaceString(comment, "\n", "\n# "); file << comment << "\n\n"; diff --git a/Source/cmakemain.cxx b/Source/cmakemain.cxx index 577dcd9f7..e5f4700b7 100644 --- a/Source/cmakemain.cxx +++ b/Source/cmakemain.cxx @@ -130,7 +130,7 @@ static std::string cmakemainGetStack(void *clientdata) cmMakefile* mf=cmakemainGetMakefile(clientdata); if (mf) { - msg = mf->GetListFileStack(); + msg = mf->FormatListFileStack(); if (!msg.empty()) { msg = "\n Called from: " + msg; From 390bc3244fd94d687bd48860eb2a70ad9674d755 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 2 May 2015 10:25:13 +0200 Subject: [PATCH 05/18] cmMakefile: Remove redundant condition. As this is called in the constructor, the definition will never be already set. --- Source/cmMakefile.cxx | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index c1bbd12a3..e84ea4e51 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -230,18 +230,12 @@ cmMakefile::cmMakefile(cmLocalGenerator* localGenerator) { const char* dir = this->GetCMakeInstance()->GetHomeDirectory(); this->AddDefinition("CMAKE_SOURCE_DIR", dir); - if ( !this->GetDefinition("CMAKE_CURRENT_SOURCE_DIR") ) - { - this->AddDefinition("CMAKE_CURRENT_SOURCE_DIR", dir); - } + this->AddDefinition("CMAKE_CURRENT_SOURCE_DIR", dir); } { const char* dir = this->GetCMakeInstance()->GetHomeOutputDirectory(); this->AddDefinition("CMAKE_BINARY_DIR", dir); - if ( !this->GetDefinition("CMAKE_CURRENT_BINARY_DIR") ) - { - this->AddDefinition("CMAKE_CURRENT_BINARY_DIR", dir); - } + this->AddDefinition("CMAKE_CURRENT_BINARY_DIR", dir); } } From 5b7ff35c4d8b41e3d08305b91b0e5973f2e3906b Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 16 May 2015 07:29:36 +0200 Subject: [PATCH 06/18] cmMakefile: Don't expect the VarStack iterator to support size(). --- Source/cmMakefile.cxx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index e84ea4e51..1a192f5f0 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -105,9 +105,8 @@ public: bool RaiseScope(std::string const& var, const char* varDef, cmMakefile* mf) { - assert(this->VarStack.size() > 0); - std::list::reverse_iterator it = this->VarStack.rbegin(); + assert(it != this->VarStack.rend()); ++it; if(it == this->VarStack.rend()) { From 1363bff83a8e525814bbcfd1e90173d392ecc52b Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 16 May 2015 16:39:36 +0200 Subject: [PATCH 07/18] cmMakefile: Remove duplicate variable initialization. --- Source/cmMakefile.cxx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 1a192f5f0..a95f4b836 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -152,8 +152,8 @@ cmMakefile::cmMakefile(cmLocalGenerator* localGenerator) this->Internal->IsSourceFileTryCompile = false; // Initialize these first since AddDefaultDefinitions calls AddDefinition - this->WarnUnused = false; - this->CheckSystemVars = false; + this->WarnUnused = this->GetCMakeInstance()->GetWarnUnused(); + this->CheckSystemVars = this->GetCMakeInstance()->GetCheckSystemVars(); this->GeneratingBuildSystem = false; this->SuppressWatches = false; @@ -223,8 +223,6 @@ cmMakefile::cmMakefile(cmLocalGenerator* localGenerator) #endif this->Properties.SetCMakeInstance(this->GetCMakeInstance()); - this->WarnUnused = this->GetCMakeInstance()->GetWarnUnused(); - this->CheckSystemVars = this->GetCMakeInstance()->GetCheckSystemVars(); { const char* dir = this->GetCMakeInstance()->GetHomeDirectory(); From bb1e8c3adfc4b188b0f340d757591039ffda4ef9 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 16 May 2015 16:43:17 +0200 Subject: [PATCH 08/18] cmMakefile: Remove Print() debugging facilities. They don't print things that are important in the modern implementation. --- Source/cmMakefile.cxx | 53 ------------------------------------------- Source/cmMakefile.h | 9 -------- 2 files changed, 62 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index a95f4b836..1eadecf24 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -253,59 +253,6 @@ cmMakefile::~cmMakefile() } } -void cmMakefile::PrintStringVector(const char* s, - const std::vector& v) const -{ - std::cout << s << ": ( \n" << cmWrap('"', v, '"', " ") << ")\n"; -} - -void cmMakefile -::PrintStringVector(const char* s, - const std::vector >& v) const -{ - std::cout << s << ": ( \n"; - for(std::vector >::const_iterator i - = v.begin(); i != v.end(); ++i) - { - std::cout << i->first << " " << i->second; - } - std::cout << " )\n"; -} - - -// call print on all the classes in the makefile -void cmMakefile::Print() const -{ - // print the class lists - std::cout << "classes:\n"; - - std::cout << " this->Targets: "; - for (cmTargets::iterator l = this->Targets.begin(); - l != this->Targets.end(); l++) - { - std::cout << l->first << std::endl; - } - - std::cout << " this->StartOutputDirectory; " << - this->GetCurrentBinaryDirectory() << std::endl; - std::cout << " this->HomeOutputDirectory; " << - this->GetHomeOutputDirectory() << std::endl; - std::cout << " this->cmStartDirectory; " << - this->GetCurrentSourceDirectory() << std::endl; - std::cout << " this->cmHomeDirectory; " << - this->GetHomeDirectory() << std::endl; - std::cout << " this->ProjectName; " - << this->ProjectName << std::endl; - this->PrintStringVector("this->LinkDirectories", this->LinkDirectories); -#if defined(CMAKE_BUILD_WITH_CMAKE) - for( std::vector::const_iterator i = - this->SourceGroups.begin(); i != this->SourceGroups.end(); ++i) - { - std::cout << "Source Group: " << i->GetName() << std::endl; - } -#endif -} - //---------------------------------------------------------------------------- void cmMakefile::IssueMessage(cmake::MessageType t, std::string const& text) const diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index 72d8ec6d4..f96ca92f2 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -157,11 +157,6 @@ public: */ void FinalPass(); - /** - * Print the object state to std::cout. - */ - void Print() const; - /** Add a custom command to the build. */ void AddCustomCommandToTarget(const std::string& target, const std::vector& byproducts, @@ -912,10 +907,6 @@ private: friend class cmMakeDepend; // make depend needs direct access // to the Sources array - void PrintStringVector(const char* s, const - std::vector >& v) const; - void PrintStringVector(const char* s, - const std::vector& v) const; void AddDefaultDefinitions(); typedef std::vector FunctionBlockersType; From caff8e5a3e03c7841a35e563c014322f26611aef Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sat, 16 May 2015 16:46:12 +0200 Subject: [PATCH 09/18] cmCTest: Remove unimplemented method. --- Source/cmTest.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Source/cmTest.h b/Source/cmTest.h index c6e7e42e8..ca88afecd 100644 --- a/Source/cmTest.h +++ b/Source/cmTest.h @@ -40,11 +40,6 @@ public: return this->Command; } - /** - * Print the structure to std::cout. - */ - void Print() const; - ///! Set/Get a property of this source file void SetProperty(const std::string& prop, const char *value); void AppendProperty(const std::string& prop, From c42f0e2b3e7d3b8eed0ef0ab5a973fe9375339b1 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 12:07:10 +0200 Subject: [PATCH 10/18] cmMakefile: Remove redundant conditions. This container is never empty. --- Source/cmMakefile.cxx | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 1eadecf24..56766dcd5 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -1730,8 +1730,7 @@ void cmMakefile::AddDefinition(const std::string& name, const char* value) } this->Internal->SetDefinition(name, value); - if (!this->Internal->VarUsageStack.empty() && - this->VariableInitialized(name)) + if (this->VariableInitialized(name)) { this->CheckForUnused("changing definition", name); this->Internal->VarUsageStack.top().erase(name); @@ -1806,8 +1805,7 @@ void cmMakefile::AddCacheDefinition(const std::string& name, const char* value, void cmMakefile::AddDefinition(const std::string& name, bool value) { this->Internal->SetDefinition(name, value ? "ON" : "OFF"); - if (!this->Internal->VarUsageStack.empty() && - this->VariableInitialized(name)) + if (this->VariableInitialized(name)) { this->CheckForUnused("changing definition", name); this->Internal->VarUsageStack.top().erase(name); @@ -1904,8 +1902,7 @@ void cmMakefile::CheckForUnused(const char* reason, void cmMakefile::RemoveDefinition(const std::string& name) { this->Internal->RemoveDefinition(name); - if (!this->Internal->VarUsageStack.empty() && - this->VariableInitialized(name)) + if (this->VariableInitialized(name)) { this->CheckForUnused("unsetting", name); this->Internal->VarUsageStack.top().erase(name); From bdd1aa91ae28f5df7193e724b1351c616e56dc0a Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 13:20:34 +0200 Subject: [PATCH 11/18] cmMakefile: Don't use else after return. --- Source/cmMakefile.cxx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 56766dcd5..5be6b1bc0 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -126,10 +126,7 @@ public: } return true; } - else - { - return false; - } + return false; } // First localize the definition in the current scope. this->GetDefinition(var); From c8cb66880c233414b6656ea3d23776f9ba07a4e4 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 13:24:18 +0200 Subject: [PATCH 12/18] cmMakefile: Use early return to reduce nested code. --- Source/cmMakefile.cxx | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 5be6b1bc0..21c9f47dd 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -110,23 +110,24 @@ public: ++it; if(it == this->VarStack.rend()) { - if(cmLocalGenerator* plg = mf->GetLocalGenerator()->GetParent()) + cmLocalGenerator* plg = mf->GetLocalGenerator()->GetParent(); + if(!plg) { - // 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); - } - return true; + return false; } - return false; + // 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); + } + return true; } // First localize the definition in the current scope. this->GetDefinition(var); From ea7b962be2f157f60f143725948e56b2f9f07042 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 13:21:02 +0200 Subject: [PATCH 13/18] cmMakefile: Raise variable in scope explicitly when needed. The Get method implicitly pulls a copy of all variables into a local scope. This is not necessary. --- Source/cmDefinitions.cxx | 16 +++++++++++++--- Source/cmDefinitions.h | 6 +++--- Source/cmMakefile.cxx | 2 +- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index e2c687632..d7b6279a9 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -18,7 +18,7 @@ cmDefinitions::Def cmDefinitions::NoDef; //---------------------------------------------------------------------------- cmDefinitions::Def const& cmDefinitions::GetInternal( - const std::string& key, StackIter begin, StackIter end) + const std::string& key, StackIter begin, StackIter end, bool raise) { assert(begin != end); MapType::const_iterator i = begin->Map.find(key); @@ -32,7 +32,11 @@ cmDefinitions::Def const& cmDefinitions::GetInternal( { return cmDefinitions::NoDef; } - Def const& def = cmDefinitions::GetInternal(key, it, end); + Def const& def = cmDefinitions::GetInternal(key, it, end, raise); + if (!raise) + { + return def; + } return begin->Map.insert(MapType::value_type(key, def)).first->second; } @@ -40,10 +44,16 @@ cmDefinitions::Def const& cmDefinitions::GetInternal( const char* cmDefinitions::Get(const std::string& key, StackIter begin, StackIter end) { - Def const& def = cmDefinitions::GetInternal(key, begin, end); + Def const& def = cmDefinitions::GetInternal(key, begin, end, false); return def.Exists? def.c_str() : 0; } +void cmDefinitions::Raise(const std::string& key, + StackIter begin, StackIter end) +{ + cmDefinitions::GetInternal(key, begin, end, true); +} + //---------------------------------------------------------------------------- void cmDefinitions::Set(const std::string& key, const char* value) { diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index bf791edea..17b9c7cde 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -35,11 +35,11 @@ class cmDefinitions typedef std::list::reverse_iterator StackIter; typedef std::list::const_reverse_iterator StackConstIter; public: - /** Get the value associated with a key; null if none. - Store the result locally if it came from a parent. */ static const char* Get(const std::string& key, StackIter begin, StackIter end); + static void Raise(const std::string& key, StackIter begin, StackIter end); + /** Set (or unset if null) a value associated with a key. */ void Set(const std::string& key, const char* value); @@ -80,7 +80,7 @@ private: MapType Map; static Def const& GetInternal(const std::string& key, - StackIter begin, StackIter end); + StackIter begin, StackIter end, bool raise); }; #endif diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 21c9f47dd..adba110f7 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -130,7 +130,7 @@ public: return true; } // First localize the definition in the current scope. - this->GetDefinition(var); + cmDefinitions::Raise(var, this->VarStack.rbegin(), this->VarStack.rend()); // Now update the definition in the parent scope. it->Set(var, varDef); From f58c3774d15f16ea287ca52fcbd04c17f0a5612d Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 15:20:38 +0200 Subject: [PATCH 14/18] cmMakefile: Mark definitions explicitly erased, even at top level. Presumably the intention here is to attempt to optimize memory by not storing what is not needed. However, all keys need to be tracked anyway to implement initialization tracking, and this special case gets in the way of simplifying the implementation of that. This doesn't change any observable effects because values set to 0 are considered not to exist by the cmDefinitions API. --- Source/cmDefinitions.cxx | 5 ----- Source/cmDefinitions.h | 2 -- Source/cmMakefile.cxx | 10 +--------- 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index d7b6279a9..97a16ea23 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -61,11 +61,6 @@ void cmDefinitions::Set(const std::string& key, const char* value) this->Map[key] = def; } -void cmDefinitions::Erase(const std::string& key) -{ - this->Map.erase(key); -} - //---------------------------------------------------------------------------- std::vector cmDefinitions::LocalKeys() const { diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 17b9c7cde..894ff7a02 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -43,8 +43,6 @@ 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::vector LocalKeys() const; diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index adba110f7..ad48bb71f 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -76,15 +76,7 @@ public: void RemoveDefinition(std::string const& name) { - if (this->VarStack.size() > 1) - { - // In lower scopes we store keys, defined or not. - this->VarStack.back().Set(name, 0); - } - else - { - this->VarStack.back().Erase(name); - } + this->VarStack.back().Set(name, 0); } std::vector LocalKeys() const From 9118b53b79afc1791d920b8ea6f9cedf26010148 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 16:27:47 +0200 Subject: [PATCH 15/18] cmMakefile: Move internal method to private scope. --- Source/cmMakefile.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index f96ca92f2..90544721d 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -65,8 +65,6 @@ class cmMakefile class Internals; cmsys::auto_ptr Internal; public: - /* Check for unused variables in this scope */ - void CheckForUnusedVariables() const; /* Mark a variable as used */ void MarkVariableAsUsed(const std::string& var); /* return true if a variable has been initialized */ @@ -1042,6 +1040,8 @@ private: bool HaveCxxStandardAvailable(cmTarget const* target, const std::string& feature) const; + void CheckForUnusedVariables() const; + mutable bool SuppressWatches; }; From 528d68021c6769b2aa86ea9751a7308a84101ca2 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 16:28:39 +0200 Subject: [PATCH 16/18] cmMakefile: Use more suitable method name to log var usage. --- Source/cmMakefile.cxx | 12 ++++++------ Source/cmMakefile.h | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index ad48bb71f..645a4332f 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -1722,7 +1722,7 @@ void cmMakefile::AddDefinition(const std::string& name, const char* value) this->Internal->SetDefinition(name, value); if (this->VariableInitialized(name)) { - this->CheckForUnused("changing definition", name); + this->LogUnused("changing definition", name); this->Internal->VarUsageStack.top().erase(name); } this->Internal->VarInitStack.top().insert(name); @@ -1797,7 +1797,7 @@ void cmMakefile::AddDefinition(const std::string& name, bool value) this->Internal->SetDefinition(name, value ? "ON" : "OFF"); if (this->VariableInitialized(name)) { - this->CheckForUnused("changing definition", name); + this->LogUnused("changing definition", name); this->Internal->VarUsageStack.top().erase(name); } this->Internal->VarInitStack.top().insert(name); @@ -1821,7 +1821,7 @@ void cmMakefile::CheckForUnusedVariables() const std::vector::const_iterator it = locals.begin(); for (; it != locals.end(); ++it) { - this->CheckForUnused("out of scope", *it); + this->LogUnused("out of scope", *it); } } @@ -1850,7 +1850,7 @@ bool cmMakefile::VariableUsed(const std::string& var) const return false; } -void cmMakefile::CheckForUnused(const char* reason, +void cmMakefile::LogUnused(const char* reason, const std::string& name) const { if (this->WarnUnused && !this->VariableUsed(name)) @@ -1894,7 +1894,7 @@ void cmMakefile::RemoveDefinition(const std::string& name) this->Internal->RemoveDefinition(name); if (this->VariableInitialized(name)) { - this->CheckForUnused("unsetting", name); + this->LogUnused("unsetting", name); this->Internal->VarUsageStack.top().erase(name); } this->Internal->VarInitStack.top().insert(name); @@ -4347,7 +4347,7 @@ void cmMakefile::PopScope() init.erase(*it); if (!this->VariableUsed(*it)) { - this->CheckForUnused("out of scope", *it); + this->LogUnused("out of scope", *it); } else { diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index 90544721d..a8873ff53 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -823,7 +823,7 @@ protected: void AddGlobalLinkInformation(const std::string& name, cmTarget& target); // Check for a an unused variable - void CheckForUnused(const char* reason, const std::string& name) const; + void LogUnused(const char* reason, const std::string& name) const; std::string ProjectName; // project name From 2b09d9f346bd3220b059771a6da1bafb06ce0f5b Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 13:37:41 +0200 Subject: [PATCH 17/18] cmMakefile: Remove VarInitStack. In cmMakefile::PushScope, a copy of the closure of keys initialized in the parent scope is made. In PopScope, essentially the same copy is inserted back into the parent. That means a lot of duplication of strings and a lot of string comparisons. None of it is needed, because the cmDefinitions keys already provide a canonical representation of what is initialized. The removal of the separate container also makes the variable handling code more easy to reason about in general. Before this patch, configuring llvm uses 200 KiB for the VarInitStack. Overall peak memory consumption goes from 35.5 MiB to 35.1 MiB. --- Source/cmDefinitions.cxx | 14 ++++++++++++++ Source/cmDefinitions.h | 3 +++ Source/cmMakefile.cxx | 32 +++++++++++--------------------- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 97a16ea23..5b03bd43a 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -54,6 +54,20 @@ void cmDefinitions::Raise(const std::string& key, cmDefinitions::GetInternal(key, begin, end, true); } +bool cmDefinitions::HasKey(const std::string& key, + StackConstIter begin, StackConstIter end) +{ + for (StackConstIter it = begin; it != end; ++it) + { + MapType::const_iterator i = it->Map.find(key); + if (i != it->Map.end()) + { + return true; + } + } + return false; +} + //---------------------------------------------------------------------------- void cmDefinitions::Set(const std::string& key, const char* value) { diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index 894ff7a02..bd3d392fc 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -40,6 +40,9 @@ public: static void Raise(const std::string& key, StackIter begin, StackIter end); + static bool HasKey(const std::string& key, + StackConstIter begin, StackConstIter end); + /** Set (or unset if null) a value associated with a key. */ void Set(const std::string& key, const char* value); diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 645a4332f..bad4e6a85 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -47,7 +47,6 @@ class cmMakefile::Internals { public: std::list VarStack; - std::stack > VarInitStack; std::stack > VarUsageStack; bool IsSourceFileTryCompile; @@ -69,6 +68,12 @@ public: this->VarStack.rend()); } + bool IsInitialized(std::string const& name) + { + return cmDefinitions::HasKey(name, this->VarStack.rbegin(), + this->VarStack.rend()); + } + void SetDefinition(std::string const& name, std::string const& value) { this->VarStack.back().Set(name, value.c_str()); @@ -137,7 +142,6 @@ cmMakefile::cmMakefile(cmLocalGenerator* localGenerator) StateSnapshot(localGenerator->GetStateSnapshot()) { this->Internal->PushDefinitions(); - this->Internal->VarInitStack.push(std::set()); this->Internal->VarUsageStack.push(std::set()); this->Internal->IsSourceFileTryCompile = false; @@ -1719,13 +1723,12 @@ void cmMakefile::AddDefinition(const std::string& name, const char* value) return; } - this->Internal->SetDefinition(name, value); if (this->VariableInitialized(name)) { this->LogUnused("changing definition", name); this->Internal->VarUsageStack.top().erase(name); } - this->Internal->VarInitStack.top().insert(name); + this->Internal->SetDefinition(name, value); #ifdef CMAKE_BUILD_WITH_CMAKE cmVariableWatch* vv = this->GetVariableWatch(); @@ -1794,13 +1797,12 @@ void cmMakefile::AddCacheDefinition(const std::string& name, const char* value, void cmMakefile::AddDefinition(const std::string& name, bool value) { - this->Internal->SetDefinition(name, value ? "ON" : "OFF"); if (this->VariableInitialized(name)) { this->LogUnused("changing definition", name); this->Internal->VarUsageStack.top().erase(name); } - this->Internal->VarInitStack.top().insert(name); + this->Internal->SetDefinition(name, value ? "ON" : "OFF"); #ifdef CMAKE_BUILD_WITH_CMAKE cmVariableWatch* vv = this->GetVariableWatch(); if ( vv ) @@ -1832,12 +1834,7 @@ void cmMakefile::MarkVariableAsUsed(const std::string& var) bool cmMakefile::VariableInitialized(const std::string& var) const { - if(this->Internal->VarInitStack.top().find(var) != - this->Internal->VarInitStack.top().end()) - { - return true; - } - return false; + return this->Internal->IsInitialized(var); } bool cmMakefile::VariableUsed(const std::string& var) const @@ -1891,13 +1888,12 @@ void cmMakefile::LogUnused(const char* reason, void cmMakefile::RemoveDefinition(const std::string& name) { - this->Internal->RemoveDefinition(name); if (this->VariableInitialized(name)) { this->LogUnused("unsetting", name); this->Internal->VarUsageStack.top().erase(name); } - this->Internal->VarInitStack.top().insert(name); + this->Internal->RemoveDefinition(name); #ifdef CMAKE_BUILD_WITH_CMAKE cmVariableWatch* vv = this->GetVariableWatch(); if ( vv ) @@ -4316,9 +4312,7 @@ std::string cmMakefile::FormatListFileStack() const void cmMakefile::PushScope() { this->Internal->PushDefinitions(); - const std::set& init = this->Internal->VarInitStack.top(); const std::set& usage = this->Internal->VarUsageStack.top(); - this->Internal->VarInitStack.push(init); this->Internal->VarUsageStack.push(usage); this->PushLoopBlockBarrier(); @@ -4336,7 +4330,6 @@ void cmMakefile::PopScope() this->PopLoopBlockBarrier(); - std::set init = this->Internal->VarInitStack.top(); std::set usage = this->Internal->VarUsageStack.top(); const std::vector& locals = this->Internal->LocalKeys(); // Remove initialization and usage information for variables in the local @@ -4344,7 +4337,6 @@ void cmMakefile::PopScope() std::vector::const_iterator it = locals.begin(); for (; it != locals.end(); ++it) { - init.erase(*it); if (!this->VariableUsed(*it)) { this->LogUnused("out of scope", *it); @@ -4356,10 +4348,8 @@ void cmMakefile::PopScope() } this->Internal->PopDefinitions(); - this->Internal->VarInitStack.pop(); this->Internal->VarUsageStack.pop(); - // Push initialization and usage up to the parent scope. - this->Internal->VarInitStack.top().insert(init.begin(), init.end()); + // Push usage up to the parent scope. this->Internal->VarUsageStack.top().insert(usage.begin(), usage.end()); } From b9f9915516c9b426f4f528bb1ec5a79d115e21ab Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 17 May 2015 16:19:55 +0200 Subject: [PATCH 18/18] cmMakefile: Remove VarUsageStack. Store usage information in the cmDefintions value instead. We already pay for the memory as padding in the Def struct, so we might as well use it. --- Source/cmDefinitions.cxx | 7 ++--- Source/cmDefinitions.h | 16 ++++++----- Source/cmMakefile.cxx | 58 ++++++---------------------------------- Source/cmMakefile.h | 2 -- 4 files changed, 22 insertions(+), 61 deletions(-) diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx index 5b03bd43a..2dab169ef 100644 --- a/Source/cmDefinitions.cxx +++ b/Source/cmDefinitions.cxx @@ -21,9 +21,10 @@ cmDefinitions::Def const& cmDefinitions::GetInternal( const std::string& key, StackIter begin, StackIter end, bool raise) { assert(begin != end); - MapType::const_iterator i = begin->Map.find(key); + MapType::iterator i = begin->Map.find(key); if (i != begin->Map.end()) { + i->second.Used = true; return i->second; } StackIter it = begin; @@ -76,7 +77,7 @@ void cmDefinitions::Set(const std::string& key, const char* value) } //---------------------------------------------------------------------------- -std::vector cmDefinitions::LocalKeys() const +std::vector cmDefinitions::UnusedKeys() const { std::vector keys; keys.reserve(this->Map.size()); @@ -84,7 +85,7 @@ std::vector cmDefinitions::LocalKeys() const for(MapType::const_iterator mi = this->Map.begin(); mi != this->Map.end(); ++mi) { - if (mi->second.Exists) + if (!mi->second.Used) { keys.push_back(mi->first); } diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h index bd3d392fc..5fdcaab72 100644 --- a/Source/cmDefinitions.h +++ b/Source/cmDefinitions.h @@ -46,8 +46,7 @@ public: /** Set (or unset if null) a value associated with a key. */ void Set(const std::string& key, const char* value); - /** Get the set of all local keys. */ - std::vector LocalKeys() const; + std::vector UnusedKeys() const; static std::vector ClosureKeys(StackConstIter begin, StackConstIter end); @@ -61,11 +60,16 @@ private: private: typedef std::string std_string; public: - Def(): std_string(), Exists(false) {} - Def(const char* v): std_string(v?v:""), Exists(v?true:false) {} - Def(const std_string& v): std_string(v), Exists(true) {} - Def(Def const& d): std_string(d), Exists(d.Exists) {} + Def(): std_string(), Exists(false), Used(false) {} + Def(const char* v) + : std_string(v ? v : ""), + Exists(v ? true : false), + Used(false) + {} + Def(const std_string& v): std_string(v), Exists(true), Used(false) {} + Def(Def const& d): std_string(d), Exists(d.Exists), Used(d.Used) {} bool Exists; + bool Used; }; static Def NoDef; diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index bad4e6a85..31ab66dbe 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -38,7 +38,6 @@ #include #include -#include #include #include // for isspace #include @@ -47,7 +46,6 @@ class cmMakefile::Internals { public: std::list VarStack; - std::stack > VarUsageStack; bool IsSourceFileTryCompile; void PushDefinitions() @@ -84,9 +82,9 @@ public: this->VarStack.back().Set(name, 0); } - std::vector LocalKeys() const + std::vector UnusedKeys() const { - return this->VarStack.back().LocalKeys(); + return this->VarStack.back().UnusedKeys(); } std::vector ClosureKeys() const @@ -142,7 +140,6 @@ cmMakefile::cmMakefile(cmLocalGenerator* localGenerator) StateSnapshot(localGenerator->GetStateSnapshot()) { this->Internal->PushDefinitions(); - this->Internal->VarUsageStack.push(std::set()); this->Internal->IsSourceFileTryCompile = false; // Initialize these first since AddDefaultDefinitions calls AddDefinition @@ -588,7 +585,6 @@ bool cmMakefile::ReadListFile(const char* listfile, if (res) { - // Check for unused variables this->CheckForUnusedVariables(); } @@ -1726,7 +1722,6 @@ void cmMakefile::AddDefinition(const std::string& name, const char* value) if (this->VariableInitialized(name)) { this->LogUnused("changing definition", name); - this->Internal->VarUsageStack.top().erase(name); } this->Internal->SetDefinition(name, value); @@ -1800,7 +1795,6 @@ void cmMakefile::AddDefinition(const std::string& name, bool value) if (this->VariableInitialized(name)) { this->LogUnused("changing definition", name); - this->Internal->VarUsageStack.top().erase(name); } this->Internal->SetDefinition(name, value ? "ON" : "OFF"); #ifdef CMAKE_BUILD_WITH_CMAKE @@ -1819,9 +1813,9 @@ void cmMakefile::CheckForUnusedVariables() const { return; } - const std::vector& locals = this->Internal->LocalKeys(); - std::vector::const_iterator it = locals.begin(); - for (; it != locals.end(); ++it) + const std::vector& unused = this->Internal->UnusedKeys(); + std::vector::const_iterator it = unused.begin(); + for (; it != unused.end(); ++it) { this->LogUnused("out of scope", *it); } @@ -1829,7 +1823,7 @@ void cmMakefile::CheckForUnusedVariables() const void cmMakefile::MarkVariableAsUsed(const std::string& var) { - this->Internal->VarUsageStack.top().insert(var); + this->Internal->GetDefinition(var); } bool cmMakefile::VariableInitialized(const std::string& var) const @@ -1837,20 +1831,10 @@ bool cmMakefile::VariableInitialized(const std::string& var) const return this->Internal->IsInitialized(var); } -bool cmMakefile::VariableUsed(const std::string& var) const -{ - if(this->Internal->VarUsageStack.top().find(var) != - this->Internal->VarUsageStack.top().end()) - { - return true; - } - return false; -} - void cmMakefile::LogUnused(const char* reason, const std::string& name) const { - if (this->WarnUnused && !this->VariableUsed(name)) + if (this->WarnUnused) { std::string path; cmListFileBacktrace bt(this->GetLocalGenerator()); @@ -1891,7 +1875,6 @@ void cmMakefile::RemoveDefinition(const std::string& name) if (this->VariableInitialized(name)) { this->LogUnused("unsetting", name); - this->Internal->VarUsageStack.top().erase(name); } this->Internal->RemoveDefinition(name); #ifdef CMAKE_BUILD_WITH_CMAKE @@ -2360,7 +2343,6 @@ const char* cmMakefile::GetRequiredDefinition(const std::string& name) const bool cmMakefile::IsDefinitionSet(const std::string& name) const { const char* def = this->Internal->GetDefinition(name); - this->Internal->VarUsageStack.top().insert(name); if(!def) { def = this->GetState()->GetInitializedCacheValue(name); @@ -2381,10 +2363,6 @@ bool cmMakefile::IsDefinitionSet(const std::string& name) const const char* cmMakefile::GetDefinition(const std::string& name) const { - if (this->WarnUnused) - { - this->Internal->VarUsageStack.top().insert(name); - } const char* def = this->Internal->GetDefinition(name); if(!def) { @@ -4312,8 +4290,6 @@ std::string cmMakefile::FormatListFileStack() const void cmMakefile::PushScope() { this->Internal->PushDefinitions(); - const std::set& usage = this->Internal->VarUsageStack.top(); - this->Internal->VarUsageStack.push(usage); this->PushLoopBlockBarrier(); @@ -4330,27 +4306,9 @@ void cmMakefile::PopScope() this->PopLoopBlockBarrier(); - std::set usage = this->Internal->VarUsageStack.top(); - const std::vector& locals = this->Internal->LocalKeys(); - // Remove initialization and usage information for variables in the local - // scope. - std::vector::const_iterator it = locals.begin(); - for (; it != locals.end(); ++it) - { - if (!this->VariableUsed(*it)) - { - this->LogUnused("out of scope", *it); - } - else - { - usage.erase(*it); - } - } + this->CheckForUnusedVariables(); this->Internal->PopDefinitions(); - this->Internal->VarUsageStack.pop(); - // Push usage up to the parent scope. - this->Internal->VarUsageStack.top().insert(usage.begin(), usage.end()); } void cmMakefile::RaiseScope(const std::string& var, const char *varDef) diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index a8873ff53..efd73a1cd 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -69,8 +69,6 @@ public: void MarkVariableAsUsed(const std::string& var); /* return true if a variable has been initialized */ bool VariableInitialized(const std::string& ) const; - /* return true if a variable has been used */ - bool VariableUsed(const std::string& ) const; /** * Construct an empty makefile.