cmFunctionBlocker: Move check for matching args

9 files changed, 65 insertions, 100 deletions

diff --git a/Source/cmForEachCommand.cxx b/Source/cmForEachCommand.cxx
index 53ae479..10ce459 100644
--- a/Source/cmForEachCommand.cxx
+++ b/Source/cmForEachCommand.cxx
@@ -28,7 +28,9 @@ public:
   cm::string_view StartCommandName() const override { return "foreach"_s; }
   cm::string_view EndCommandName() const override { return "endforeach"_s; }
 
-  bool ShouldRemove(const cmListFileFunction& lff, cmMakefile& mf) override;
+  bool ArgumentsMatch(cmListFileFunction const& lff,
+                      cmMakefile& mf) const override;
+
   bool Replay(std::vector<cmListFileFunction> const& functions,
               cmExecutionStatus& inStatus) override;
 
@@ -49,20 +51,12 @@ cmForEachFunctionBlocker::~cmForEachFunctionBlocker()
   this->Makefile->PopLoopBlock();
 }
 
-bool cmForEachFunctionBlocker::ShouldRemove(const cmListFileFunction& lff,
-                                            cmMakefile& mf)
+bool cmForEachFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff,
+                                              cmMakefile& mf) const
 {
-  if (lff.Name.Lower == "endforeach") {
-    std::vector<std::string> expandedArguments;
-    mf.ExpandArguments(lff.Arguments, expandedArguments);
-    // if the endforeach has arguments then make sure
-    // they match the begin foreach arguments
-    if ((expandedArguments.empty() ||
-         (expandedArguments[0] == this->Args[0]))) {
-      return true;
-    }
-  }
-  return false;
+  std::vector<std::string> expandedArguments;
+  mf.ExpandArguments(lff.Arguments, expandedArguments);
+  return expandedArguments.empty() || expandedArguments[0] == this->Args[0];
 }
 
 bool cmForEachFunctionBlocker::Replay(
diff --git a/Source/cmFunctionBlocker.cxx b/Source/cmFunctionBlocker.cxx
index 2999691..437d4b5 100644
--- a/Source/cmFunctionBlocker.cxx
+++ b/Source/cmFunctionBlocker.cxx
@@ -2,8 +2,12 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmFunctionBlocker.h"
 
+#include <cassert>
+#include <sstream>
+
 #include "cmExecutionStatus.h"
 #include "cmMakefile.h"
+#include "cmMessageType.h"
 
 bool cmFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff,
                                           cmExecutionStatus& status)
@@ -14,7 +18,24 @@ bool cmFunctionBlocker::IsFunctionBlocked(const cmListFileFunction& lff,
     this->ScopeDepth--;
     if (this->ScopeDepth == 0U) {
       cmMakefile& mf = status.GetMakefile();
-      auto self = mf.RemoveFunctionBlocker(this, lff);
+      auto self = mf.RemoveFunctionBlocker();
+      assert(self.get() == this);
+
+      if (!this->ArgumentsMatch(lff, mf)) {
+        cmListFileContext const& lfc = this->GetStartingContext();
+        cmListFileContext closingContext =
+          cmListFileContext::FromCommandContext(lff, lfc.FilePath);
+        std::ostringstream e;
+        /* clang-format off */
+        e << "A logical block opening on the line\n"
+          << "  " << lfc << "\n"
+          << "closes on the line\n"
+          << "  " << closingContext << "\n"
+          << "with mis-matching arguments.";
+        /* clang-format on */
+        mf.IssueMessage(MessageType::AUTHOR_WARNING, e.str());
+      }
+
       return this->Replay(this->Functions, status);
     }
   }
diff --git a/Source/cmFunctionBlocker.h b/Source/cmFunctionBlocker.h
index 8fc2c1c..8b05993 100644
--- a/Source/cmFunctionBlocker.h
+++ b/Source/cmFunctionBlocker.h
@@ -23,15 +23,6 @@ public:
   bool IsFunctionBlocked(cmListFileFunction const& lff,
                          cmExecutionStatus& status);
 
-  /**
-   * should this function blocker be removed, useful when one function adds a
-   * blocker and another must remove it
-   */
-  virtual bool ShouldRemove(const cmListFileFunction&, cmMakefile&)
-  {
-    return false;
-  }
-
   virtual ~cmFunctionBlocker() = default;
 
   /** Set/Get the context in which this blocker is created.  */
@@ -48,6 +39,9 @@ private:
   virtual cm::string_view StartCommandName() const = 0;
   virtual cm::string_view EndCommandName() const = 0;
 
+  virtual bool ArgumentsMatch(cmListFileFunction const& lff,
+                              cmMakefile& mf) const = 0;
+
   virtual bool Replay(std::vector<cmListFileFunction> const& functions,
                       cmExecutionStatus& status) = 0;
 
diff --git a/Source/cmFunctionCommand.cxx b/Source/cmFunctionCommand.cxx
index 6a04196..dd0a5d0 100644
--- a/Source/cmFunctionCommand.cxx
+++ b/Source/cmFunctionCommand.cxx
@@ -113,29 +113,22 @@ public:
   cm::string_view StartCommandName() const override { return "function"_s; }
   cm::string_view EndCommandName() const override { return "endfunction"_s; }
 
-  bool ShouldRemove(const cmListFileFunction&, cmMakefile& mf) override;
+  bool ArgumentsMatch(cmListFileFunction const&,
+                      cmMakefile& mf) const override;
+
   bool Replay(std::vector<cmListFileFunction> const& functions,
               cmExecutionStatus& status) override;
 
   std::vector<std::string> Args;
 };
 
-bool cmFunctionFunctionBlocker::ShouldRemove(const cmListFileFunction& lff,
-                                             cmMakefile& mf)
+bool cmFunctionFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff,
+                                               cmMakefile& mf) const
 {
-  if (lff.Name.Lower == "endfunction") {
-    std::vector<std::string> expandedArguments;
-    mf.ExpandArguments(lff.Arguments, expandedArguments,
-                       this->GetStartingContext().FilePath.c_str());
-    // if the endfunction has arguments then make sure
-    // they match the ones in the opening function command
-    if ((expandedArguments.empty() ||
-         (expandedArguments[0] == this->Args[0]))) {
-      return true;
-    }
-  }
-
-  return false;
+  std::vector<std::string> expandedArguments;
+  mf.ExpandArguments(lff.Arguments, expandedArguments,
+                     this->GetStartingContext().FilePath.c_str());
+  return expandedArguments.empty() || expandedArguments[0] == this->Args[0];
 }
 
 bool cmFunctionFunctionBlocker::Replay(
diff --git a/Source/cmIfCommand.cxx b/Source/cmIfCommand.cxx
index 63cdd20..418a74b 100644
--- a/Source/cmIfCommand.cxx
+++ b/Source/cmIfCommand.cxx
@@ -38,7 +38,9 @@ public:
   cm::string_view StartCommandName() const override { return "if"_s; }
   cm::string_view EndCommandName() const override { return "endif"_s; }
 
-  bool ShouldRemove(const cmListFileFunction& lff, cmMakefile& mf) override;
+  bool ArgumentsMatch(cmListFileFunction const& lff,
+                      cmMakefile&) const override;
+
   bool Replay(std::vector<cmListFileFunction> const& functions,
               cmExecutionStatus& inStatus) override;
 
@@ -48,18 +50,10 @@ public:
   bool ElseSeen = false;
 };
 
-bool cmIfFunctionBlocker::ShouldRemove(const cmListFileFunction& lff,
-                                       cmMakefile&)
+bool cmIfFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff,
+                                         cmMakefile&) const
 {
-  if (lff.Name.Lower == "endif") {
-    // if the endif has arguments, then make sure
-    // they match the arguments of the matching if
-    if (lff.Arguments.empty() || lff.Arguments == this->Args) {
-      return true;
-    }
-  }
-
-  return false;
+  return lff.Arguments.empty() || lff.Arguments == this->Args;
 }
 
 bool cmIfFunctionBlocker::Replay(
diff --git a/Source/cmMacroCommand.cxx b/Source/cmMacroCommand.cxx
index 030bb66..eb328cc 100644
--- a/Source/cmMacroCommand.cxx
+++ b/Source/cmMacroCommand.cxx
@@ -146,29 +146,22 @@ public:
   cm::string_view StartCommandName() const override { return "macro"_s; }
   cm::string_view EndCommandName() const override { return "endmacro"_s; }
 
-  bool ShouldRemove(const cmListFileFunction&, cmMakefile& mf) override;
+  bool ArgumentsMatch(cmListFileFunction const&,
+                      cmMakefile& mf) const override;
+
   bool Replay(std::vector<cmListFileFunction> const& functions,
               cmExecutionStatus& status) override;
 
   std::vector<std::string> Args;
 };
 
-bool cmMacroFunctionBlocker::ShouldRemove(const cmListFileFunction& lff,
-                                          cmMakefile& mf)
+bool cmMacroFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff,
+                                            cmMakefile& mf) const
 {
-  if (lff.Name.Lower == "endmacro") {
-    std::vector<std::string> expandedArguments;
-    mf.ExpandArguments(lff.Arguments, expandedArguments,
-                       this->GetStartingContext().FilePath.c_str());
-    // if the endmacro has arguments make sure they
-    // match the arguments of the macro
-    if ((expandedArguments.empty() ||
-         (expandedArguments[0] == this->Args[0]))) {
-      return true;
-    }
-  }
-
-  return false;
+  std::vector<std::string> expandedArguments;
+  mf.ExpandArguments(lff.Arguments, expandedArguments,
+                     this->GetStartingContext().FilePath.c_str());
+  return expandedArguments.empty() || expandedArguments[0] == this->Args[0];
 }
 
 bool cmMacroFunctionBlocker::Replay(
diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index 8c11786..015453a 100644
--- a/Source/cmMakefile.cxx
+++ b/Source/cmMakefile.cxx
@@ -3220,30 +3220,12 @@ void cmMakefile::AddFunctionBlocker(std::unique_ptr<cmFunctionBlocker> fb)
   this->FunctionBlockers.push(std::move(fb));
 }
 
-std::unique_ptr<cmFunctionBlocker> cmMakefile::RemoveFunctionBlocker(
-  cmFunctionBlocker* fb, const cmListFileFunction& lff)
+std::unique_ptr<cmFunctionBlocker> cmMakefile::RemoveFunctionBlocker()
 {
   assert(!this->FunctionBlockers.empty());
-  assert(this->FunctionBlockers.top().get() == fb);
   assert(this->FunctionBlockerBarriers.empty() ||
          this->FunctionBlockers.size() > this->FunctionBlockerBarriers.back());
 
-  // Warn if the arguments do not match, but always remove.
-  if (!fb->ShouldRemove(lff, *this)) {
-    cmListFileContext const& lfc = fb->GetStartingContext();
-    cmListFileContext closingContext =
-      cmListFileContext::FromCommandContext(lff, lfc.FilePath);
-    std::ostringstream e;
-    /* clang-format off */
-    e << "A logical block opening on the line\n"
-      << "  " << lfc << "\n"
-      << "closes on the line\n"
-      << "  " << closingContext << "\n"
-      << "with mis-matching arguments.";
-    /* clang-format on */
-    this->IssueMessage(MessageType::AUTHOR_WARNING, e.str());
-  }
-
   auto b = std::move(this->FunctionBlockers.top());
   this->FunctionBlockers.pop();
   return b;
diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h
index 941e335..4d61c05 100644
--- a/Source/cmMakefile.h
+++ b/Source/cmMakefile.h
@@ -107,8 +107,7 @@ public:
    * Remove the function blocker whose scope ends with the given command.
    * This returns ownership of the function blocker object.
    */
-  std::unique_ptr<cmFunctionBlocker> RemoveFunctionBlocker(
-    cmFunctionBlocker* fb, const cmListFileFunction& lff);
+  std::unique_ptr<cmFunctionBlocker> RemoveFunctionBlocker();
 
   /**
    * Try running cmake and building a file. This is used for dynalically
diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx
index bacee58..1e442e5 100644
--- a/Source/cmWhileCommand.cxx
+++ b/Source/cmWhileCommand.cxx
@@ -27,7 +27,9 @@ public:
   cm::string_view StartCommandName() const override { return "while"_s; }
   cm::string_view EndCommandName() const override { return "endwhile"_s; }
 
-  bool ShouldRemove(const cmListFileFunction& lff, cmMakefile& mf) override;
+  bool ArgumentsMatch(cmListFileFunction const& lff,
+                      cmMakefile& mf) const override;
+
   bool Replay(std::vector<cmListFileFunction> const& functions,
               cmExecutionStatus& inStatus) override;
 
@@ -48,17 +50,10 @@ cmWhileFunctionBlocker::~cmWhileFunctionBlocker()
   this->Makefile->PopLoopBlock();
 }
 
-bool cmWhileFunctionBlocker::ShouldRemove(const cmListFileFunction& lff,
-                                          cmMakefile&)
+bool cmWhileFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff,
+                                            cmMakefile&) const
 {
-  if (lff.Name.Lower == "endwhile") {
-    // if the endwhile has arguments, then make sure
-    // they match the arguments of the matching while
-    if (lff.Arguments.empty() || lff.Arguments == this->Args) {
-      return true;
-    }
-  }
-  return false;
+  return lff.Arguments.empty() || lff.Arguments == this->Args;
 }
 
 bool cmWhileFunctionBlocker::Replay(