ENH: Refactor logical block enforcement

This uses a stack of 'barriers' to efficiently divide function blockers
into groups corresponding to each input file.  It simplifies detection
of missing block close commands and factors it out of ReadListFile.

2 files changed, 86 insertions, 40 deletions

diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index ed70c07..34b4855 100644
--- a/Source/cmMakefile.cxx
+++ b/Source/cmMakefile.cxx
@@ -206,7 +206,7 @@ cmMakefile::~cmMakefile()
       delete d->second;
       }
     }
-  std::list<cmFunctionBlocker *>::iterator pos;
+  std::vector<cmFunctionBlocker*>::iterator pos;
   for (pos = this->FunctionBlockers.begin();
        pos != this->FunctionBlockers.end(); ++pos)
     {
@@ -453,10 +453,6 @@ bool cmMakefile::ReadListFile(const char* filename_in,
     = this->GetSafeDefinition("CMAKE_CURRENT_LIST_FILE");
   this->AddDefinition("CMAKE_PARENT_LIST_FILE", filename_in);
 
-  // used to watch for blockers going out of scope
-  // e.g. mismatched IF statement
-  std::set<cmFunctionBlocker *> originalBlockers;
-
   const char* external = 0;
   std::string external_abs;
 
@@ -487,14 +483,6 @@ bool cmMakefile::ReadListFile(const char* filename_in,
       }
     }
 
-  // loop over current function blockers and record them
-  for (std::list<cmFunctionBlocker *>::iterator pos 
-        = this->FunctionBlockers.begin();
-       pos != this->FunctionBlockers.end(); ++pos)
-    {
-    originalBlockers.insert(*pos);
-    }
-
   // Now read the input file
   const char *filenametoread= filename;
 
@@ -541,6 +529,10 @@ bool cmMakefile::ReadListFile(const char* filename_in,
     }
   // add this list file to the list of dependencies
   this->ListFiles.push_back( filenametoread);
+
+  // Enforce balanced blocks (if/endif, function/endfunction, etc.).
+  {
+  LexicalPushPop lexScope(this);
   bool endScopeNicely = true;
 
   // Save the current policy stack depth.
@@ -552,11 +544,16 @@ bool cmMakefile::ReadListFile(const char* filename_in,
     {
     cmExecutionStatus status;
     this->ExecuteCommand(cacheFile.Functions[i],status);
-    if (status.GetReturnInvoked() ||
-        cmSystemTools::GetFatalErrorOccured() )
+    if(cmSystemTools::GetFatalErrorOccured())
       {
-      // Exit early from processing this file.
+      // Exit early due to error.
       endScopeNicely = false;
+      lexScope.Quiet();
+      break;
+      }
+    if(status.GetReturnInvoked())
+      {
+      // Exit early due to return command.
       break;
       }
     }
@@ -571,23 +568,7 @@ bool cmMakefile::ReadListFile(const char* filename_in,
       }
     this->PopPolicy(false);
     }
-
-  // send scope ended to and function blockers
-  if (endScopeNicely)
-    {
-    // loop over all function blockers to see if any block this command
-    for (std::list<cmFunctionBlocker *>::iterator pos 
-         = this->FunctionBlockers.begin();
-         pos != this->FunctionBlockers.end(); ++pos)
-      {
-      // if this blocker was not in the original then send a
-      // scope ended message
-      if (originalBlockers.find(*pos) == originalBlockers.end())
-        {
-        (*pos)->ScopeEnded(*this);
-        }
-      }
-    }
+  }
 
   // If this is the directory-level CMakeLists.txt file then perform
   // some extra checks.
@@ -2353,7 +2334,7 @@ bool cmMakefile::IsFunctionBlocked(const cmListFileFunction& lff,
 
   // loop over all function blockers to see if any block this command
   // evaluate in reverse, this is critical for balanced IF statements etc
-  std::list<cmFunctionBlocker *>::reverse_iterator pos;
+  std::vector<cmFunctionBlocker*>::reverse_iterator pos;
   for (pos = this->FunctionBlockers.rbegin();
        pos != this->FunctionBlockers.rend(); ++pos)
     {
@@ -2366,6 +2347,32 @@ bool cmMakefile::IsFunctionBlocked(const cmListFileFunction& lff,
   return false;
 }
 
+//----------------------------------------------------------------------------
+void cmMakefile::PushFunctionBlockerBarrier()
+{
+  this->FunctionBlockerBarriers.push_back(this->FunctionBlockers.size());
+}
+
+//----------------------------------------------------------------------------
+void cmMakefile::PopFunctionBlockerBarrier(bool reportError)
+{
+  // Remove any extra entries pushed on the barrier.
+  FunctionBlockersType::size_type barrier =
+    this->FunctionBlockerBarriers.back();
+  while(this->FunctionBlockers.size() > barrier)
+    {
+    cmsys::auto_ptr<cmFunctionBlocker> fb(this->FunctionBlockers.back());
+    this->FunctionBlockers.pop_back();
+    if(reportError)
+      {
+      fb->ScopeEnded(*this);
+      }
+    }
+
+  // Remove the barrier.
+  this->FunctionBlockerBarriers.pop_back();
+}
+
 bool cmMakefile::ExpandArguments(
   std::vector<cmListFileArgument> const& inArgs,
   std::vector<std::string>& outArgs)
@@ -2398,15 +2405,24 @@ bool cmMakefile::ExpandArguments(
 cmsys::auto_ptr<cmFunctionBlocker>
 cmMakefile::RemoveFunctionBlocker(const cmListFileFunction& lff)
 {
-  // loop over all function blockers to see if any block this command
-  std::list<cmFunctionBlocker *>::reverse_iterator pos;
-  for (pos = this->FunctionBlockers.rbegin();
-       pos != this->FunctionBlockers.rend(); ++pos)
+  // Find the function blocker stack barrier for the current scope.
+  // We only remove a blocker whose index is not less than the barrier.
+  FunctionBlockersType::size_type barrier = 0;
+  if(!this->FunctionBlockerBarriers.empty())
     {
+    barrier = this->FunctionBlockerBarriers.back();
+    }
+
+  // Search for the function blocker whose scope this command ends.
+  for(FunctionBlockersType::size_type
+        i = this->FunctionBlockers.size(); i > barrier; --i)
+    {
+    std::vector<cmFunctionBlocker*>::iterator pos =
+      this->FunctionBlockers.begin() + (i - 1);
     if ((*pos)->ShouldRemove(lff, *this))
       {
       cmFunctionBlocker* b = *pos;
-      this->FunctionBlockers.remove(b);
+      this->FunctionBlockers.erase(pos);
       return cmsys::auto_ptr<cmFunctionBlocker>(b);
       }
     }
@@ -2414,6 +2430,19 @@ cmMakefile::RemoveFunctionBlocker(const cmListFileFunction& lff)
   return cmsys::auto_ptr<cmFunctionBlocker>();
 }
 
+//----------------------------------------------------------------------------
+cmMakefile::LexicalPushPop::LexicalPushPop(cmMakefile* mf):
+  Makefile(mf), ReportError(true)
+{
+  this->Makefile->PushFunctionBlockerBarrier();
+}
+
+//----------------------------------------------------------------------------
+cmMakefile::LexicalPushPop::~LexicalPushPop()
+{
+  this->Makefile->PopFunctionBlockerBarrier(this->ReportError);
+}
+
 void cmMakefile::SetHomeDirectory(const char* dir)
 {
   this->cmHomeDirectory = dir;
diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h
index 6da3646..c6b332b 100644
--- a/Source/cmMakefile.h
+++ b/Source/cmMakefile.h
@@ -98,6 +98,19 @@ public:
   cmsys::auto_ptr<cmFunctionBlocker>
   RemoveFunctionBlocker(const cmListFileFunction& lff);
 
+  /** Push/pop a lexical (function blocker) barrier automatically.  */
+  class LexicalPushPop
+  {
+  public:
+    LexicalPushPop(cmMakefile* mf);
+    ~LexicalPushPop();
+    void Quiet() { this->ReportError = false; }
+  private:
+    cmMakefile* Makefile;
+    bool ReportError;
+  };
+  friend class LexicalPushPop;
+
   /**
    * Try running cmake and building a file. This is used for dynalically
    * loaded commands, not as part of the usual build process.
@@ -876,7 +889,11 @@ private:
                          const std::vector<std::string>& v) const;
 
   void AddDefaultDefinitions();
-  std::list<cmFunctionBlocker *> FunctionBlockers;
+  typedef std::vector<cmFunctionBlocker*> FunctionBlockersType;
+  FunctionBlockersType FunctionBlockers;
+  std::vector<FunctionBlockersType::size_type> FunctionBlockerBarriers;
+  void PushFunctionBlockerBarrier();
+  void PopFunctionBlockerBarrier(bool reportError = true);
 
   typedef std::map<cmStdString, cmData*> DataMapType;
   DataMapType DataMap;