Merge topic 'refactor-cmDefinitions'

b48ea26a cmDefinitions: Invert conditional code.
5ccff640 cmDefinitions: Externalize looping for ClosureKeys.
f79cd99d cmDefinitions: Implement MakeClosure in terms of reverse iterators.
aa4d1ee8 cmDefinitions: Convert MakeClosure into a static method.
60becdc6 cmDefinitions: Implement MakeClosure in terms of a list of ancestors.
d858f363 cmDefinitions: Use list of cmDefinitions* to create closure.
aaaa65b6 cmMakefile: Remove stack adaptor for the VarStack.
f983d891 cmDefinitions: Replace recursion with loop.
24885d4e cmDefinitions: Replace private constructor with MakeClosure.
012a75a0 cmDefinitions: Make ClosureKeys API vector-based.
ca9fa77d cmDefinitions: Inline GetClosureKeys implementation.
78e1454e cmDefinitions: Replace ClosureKeys recursion with looping.
818bf727 cmDefinitions: Change LocalKeys to return a vector.
5067ae41 cmDefinitions: Externalize the Set logic.
60200ca5 cmDefinitions: Add an Erase method.
b43c162e cmMakefile: Use the Internal class to enclose the VarStack.

3 files changed, 179 insertions, 134 deletions

diff --git a/Source/cmDefinitions.cxx b/Source/cmDefinitions.cxx
index abb46b3..61328be 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;
 }
 
 //----------------------------------------------------------------------------
@@ -49,107 +50,86 @@ 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)
+{
+  this->Map.erase(key);
 }
 
 //----------------------------------------------------------------------------
-std::set<std::string> cmDefinitions::LocalKeys() const
+std::vector<std::string> cmDefinitions::LocalKeys() const
 {
-  std::set<std::string> keys;
+  std::vector<std::string> 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;
 }
 
 //----------------------------------------------------------------------------
-cmDefinitions cmDefinitions::Closure() const
-{
-  return cmDefinitions(ClosureTag(), this);
-}
-
-//----------------------------------------------------------------------------
-cmDefinitions::cmDefinitions(ClosureTag const&, cmDefinitions const* root):
-  Up(0)
+cmDefinitions cmDefinitions::MakeClosure(
+    std::list<cmDefinitions>::const_reverse_iterator rbegin,
+    std::list<cmDefinitions>::const_reverse_iterator rend)
 {
   std::set<std::string> undefined;
-  this->ClosureImpl(undefined, root);
+  cmDefinitions closure;
+  closure.MakeClosure(undefined, rbegin, rend);
+  return closure;
 }
 
 //----------------------------------------------------------------------------
-void cmDefinitions::ClosureImpl(std::set<std::string>& undefined,
-                                cmDefinitions const* defs)
+void
+cmDefinitions::MakeClosure(std::set<std::string>& undefined,
+    std::list<cmDefinitions>::const_reverse_iterator rbegin,
+    std::list<cmDefinitions>::const_reverse_iterator rend)
 {
-  // Consider local definitions.
-  for(MapType::const_iterator mi = defs->Map.begin();
-      mi != defs->Map.end(); ++mi)
+  for (std::list<cmDefinitions>::const_reverse_iterator it = rbegin;
+       it != rend; ++it)
     {
-    // 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 = it->Map.begin();
+        mi != it->Map.end(); ++mi)
       {
-      if(mi->second.Exists)
-        {
-        this->Map.insert(*mi);
-        }
-      else
+      // 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())
         {
-        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->ClosureImpl(undefined, up);
-    }
-}
-
-//----------------------------------------------------------------------------
-std::set<std::string> cmDefinitions::ClosureKeys() const
-{
-  std::set<std::string> defined;
-  std::set<std::string> undefined;
-  this->ClosureKeys(defined, undefined);
-  return defined;
 }
 
 //----------------------------------------------------------------------------
-void cmDefinitions::ClosureKeys(std::set<std::string>& defined,
-                                std::set<std::string>& undefined) const
+std::vector<std::string>
+cmDefinitions::ClosureKeys(std::set<std::string>& bound) const
 {
-  // Consider local definitions.
+  std::vector<std::string> defined;
+  defined.reserve(this->Map.size());
   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())
+    if(bound.insert(mi->first).second && mi->second.Exists)
       {
-      std::set<std::string>& m = mi->second.Exists? defined : undefined;
-      m.insert(mi->first);
+      defined.push_back(mi->first);
       }
     }
-
-  // Traverse parents.
-  if(cmDefinitions const* up = this->Up)
-    {
-    up->ClosureKeys(defined, undefined);
-    }
+  return defined;
 }
diff --git a/Source/cmDefinitions.h b/Source/cmDefinitions.h
index 83cd0d9..4c7f11f 100644
--- a/Source/cmDefinitions.h
+++ b/Source/cmDefinitions.h
@@ -17,6 +17,8 @@
 #include "cmsys/hash_map.hxx"
 #endif
 
+#include <list>
+
 /** \class cmDefinitions
  * \brief Store a scope of variable definitions for CMake language.
  *
@@ -40,15 +42,17 @@ 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<std::string> LocalKeys() const;
+  std::vector<std::string> LocalKeys() const;
 
-  /** Compute the closure of all defined keys with values.
-      This flattens the scope.  The result has no parent.  */
-  cmDefinitions Closure() const;
+  std::vector<std::string>
+  ClosureKeys(std::set<std::string>& bound) const;
 
-  /** Compute the set of all defined keys.  */
-  std::set<std::string> ClosureKeys() const;
+  static cmDefinitions MakeClosure(
+      std::list<cmDefinitions>::const_reverse_iterator rbegin,
+      std::list<cmDefinitions>::const_reverse_iterator rend);
 
 private:
   // String with existence boolean.
@@ -79,15 +83,9 @@ 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<std::string>& undefined,
-                   cmDefinitions const* defs);
-
-  // Implementation of ClosureKeys() method.
-  void ClosureKeys(std::set<std::string>& defined,
-                   std::set<std::string>& undefined) const;
+  void MakeClosure(std::set<std::string>& undefined,
+                   std::list<cmDefinitions>::const_reverse_iterator rbegin,
+                   std::list<cmDefinitions>::const_reverse_iterator rend);
 };
 
 #endif
diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index 9f1d107..bd42f4e 100644
--- a/Source/cmMakefile.cxx
+++ b/Source/cmMakefile.cxx
@@ -46,10 +46,107 @@
 class cmMakefile::Internals
 {
 public:
-  std::stack<cmDefinitions, std::list<cmDefinitions> > VarStack;
+  std::list<cmDefinitions> VarStack;
   std::stack<std::set<std::string> > VarInitStack;
   std::stack<std::set<std::string> > VarUsageStack;
   bool IsSourceFileTryCompile;
+
+  void PushDefinitions()
+  {
+    cmDefinitions* parent = 0;
+    if (!this->VarStack.empty())
+      {
+      parent = &this->VarStack.back();
+      }
+    this->VarStack.push_back(cmDefinitions(parent));
+  }
+
+  void InitializeDefinitions(cmMakefile* parent)
+  {
+    this->VarStack.back() =
+        cmDefinitions::MakeClosure(parent->Internal->VarStack.rbegin(),
+                                   parent->Internal->VarStack.rend());
+  }
+
+  const char* GetDefinition(std::string const& name)
+  {
+    return this->VarStack.back().Get(name);
+  }
+
+  void SetDefinition(std::string const& name, std::string const& value)
+  {
+    this->VarStack.back().Set(name, value.c_str());
+  }
+
+  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);
+      }
+  }
+
+  std::vector<std::string> LocalKeys() const
+  {
+    return this->VarStack.back().LocalKeys();
+  }
+
+  std::vector<std::string> ClosureKeys() const
+  {
+    std::vector<std::string> closureKeys;
+    std::set<std::string> bound;
+    for (std::list<cmDefinitions>::const_reverse_iterator it =
+         this->VarStack.rbegin(); it != this->VarStack.rend(); ++it)
+      {
+      std::vector<std::string> const& localKeys = it->ClosureKeys(bound);
+      closureKeys.insert(closureKeys.end(),
+                         localKeys.begin(), localKeys.end());
+      }
+    return closureKeys;
+  }
+
+  void PopDefinitions()
+  {
+    this->VarStack.pop_back();
+  }
+
+  bool RaiseScope(std::string const& var, const char* varDef, cmMakefile* mf)
+  {
+    cmDefinitions& cur = this->VarStack.back();
+    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 +156,9 @@ cmMakefile::cmMakefile(cmLocalGenerator* localGenerator)
     StateSnapshot(localGenerator->GetGlobalGenerator()
                                 ->GetCMakeInstance()->GetState())
 {
-  const cmDefinitions& defs = cmDefinitions();
-  const std::set<std::string> 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<std::string>());
+  this->Internal->VarUsageStack.push(std::set<std::string>());
   this->Internal->IsSourceFileTryCompile = false;
 
   if (this->LocalGenerator->GetParent())
@@ -1496,7 +1591,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());
@@ -1690,7 +1785,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))
     {
@@ -1760,13 +1855,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))
     {
@@ -1790,9 +1885,8 @@ void cmMakefile::CheckForUnusedVariables() const
     {
     return;
     }
-  const cmDefinitions& defs = this->Internal->VarStack.top();
-  const std::set<std::string>& locals = defs.LocalKeys();
-  std::set<std::string>::const_iterator it = locals.begin();
+  const std::vector<std::string>& locals = this->Internal->LocalKeys();
+  std::vector<std::string>::const_iterator it = locals.begin();
   for (; it != locals.end(); ++it)
     {
     this->CheckForUnused("out of scope", *it);
@@ -1865,7 +1959,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))
     {
@@ -2338,7 +2432,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)
     {
@@ -2364,7 +2458,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);
@@ -2404,9 +2498,7 @@ std::vector<std::string> cmMakefile
   std::vector<std::string> res;
   if ( !cacheonly )
     {
-    std::set<std::string> definitions =
-        this->Internal->VarStack.top().ClosureKeys();
-    res.insert(res.end(), definitions.begin(), definitions.end());
+    res = this->Internal->ClosureKeys();
     }
   std::vector<std::string> cacheKeys =
       this->GetState()->GetCacheEntryKeys();
@@ -4297,10 +4389,9 @@ std::string cmMakefile::GetListFileStack() const
 
 void cmMakefile::PushScope()
 {
-  cmDefinitions* parent = &this->Internal->VarStack.top();
+  this->Internal->PushDefinitions();
   const std::set<std::string>& init = this->Internal->VarInitStack.top();
   const std::set<std::string>& usage = this->Internal->VarUsageStack.top();
-  this->Internal->VarStack.push(cmDefinitions(parent));
   this->Internal->VarInitStack.push(init);
   this->Internal->VarUsageStack.push(usage);
 
@@ -4321,13 +4412,12 @@ void cmMakefile::PopScope()
 
   this->PopLoopBlockBarrier();
 
-  cmDefinitions* current = &this->Internal->VarStack.top();
   std::set<std::string> init = this->Internal->VarInitStack.top();
   std::set<std::string> usage = this->Internal->VarUsageStack.top();
-  const std::set<std::string>& locals = current->LocalKeys();
+  const std::vector<std::string>& locals = this->Internal->LocalKeys();
   // Remove initialization and usage information for variables in the local
   // scope.
-  std::set<std::string>::const_iterator it = locals.begin();
+  std::vector<std::string>::const_iterator it = locals.begin();
   for (; it != locals.end(); ++it)
     {
     init.erase(*it);
@@ -4340,7 +4430,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.
@@ -4355,31 +4446,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.";