Refactor: Modernize `foreach` code and fix some bugs

- fix the typo in `foreach` documentation
- fix broken `foreach(... IN ITEMS ... LISTS ...)`
- add tests of `foreach` for existed functionality and fixes

Help/command/foreach.rst

@@ -47,7 +47,7 @@ of undocumented behavior that may change in future releases.
 
 .. code-block:: cmake
 
-  foreach(loop_var IN [LISTS [<lists>]] [ITEMS [<items>]])
+  foreach(<loop_var> IN [LISTS [<lists>]] [ITEMS [<items>]])
 
 In this variant, ``<lists>`` is a whitespace or semicolon
 separated list of list-valued variables. The ``foreach``

Source/cmForEachCommand.cxx

@@ -2,7 +2,12 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmForEachCommand.h"
 
-#include <cstdio>
+#include <algorithm>
+#include <cstddef>
+// NOTE The declaration of `std::abs` has moved to `cmath` since C++17
+// See https://en.cppreference.com/w/cpp/numeric/math/abs
+// ALERT But IWYU used to lint `#include`s do not "understand"
+// conditional compilation (i.e. `#if __cplusplus >= 201703L`)
 #include <cstdlib>
 #include <utility>
 
@@ -21,8 +26,6 @@
 #include "cmSystemTools.h"
 
 namespace {
-bool HandleInMode(std::vector<std::string> const& args, cmMakefile& makefile);
-
 class cmForEachFunctionBlocker : public cmFunctionBlocker
 {
 public:
@@ -60,7 +63,8 @@ bool cmForEachFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff,
 {
   std::vector<std::string> expandedArguments;
   mf.ExpandArguments(lff.Arguments, expandedArguments);
-  return expandedArguments.empty() || expandedArguments[0] == this->Args[0];
+  return expandedArguments.empty() ||
+    expandedArguments.front() == this->Args.front();
 }
 
 bool cmForEachFunctionBlocker::Replay(
@@ -70,13 +74,13 @@ bool cmForEachFunctionBlocker::Replay(
   // at end of for each execute recorded commands
   // store the old value
   std::string oldDef;
-  if (mf.GetDefinition(this->Args[0])) {
-    oldDef = mf.GetDefinition(this->Args[0]);
+  if (mf.GetDefinition(this->Args.front())) {
+    oldDef = mf.GetDefinition(this->Args.front());
   }
 
   for (std::string const& arg : cmMakeRange(this->Args).advance(1)) {
     // set the variable to the loop value
-    mf.AddDefinition(this->Args[0], arg);
+    mf.AddDefinition(this->Args.front(), arg);
     // Invoke all the functions that were collected in the block.
     for (cmListFileFunction const& func : functions) {
       cmExecutionStatus status(mf);
@@ -84,12 +88,12 @@ bool cmForEachFunctionBlocker::Replay(
       if (status.GetReturnInvoked()) {
         inStatus.SetReturnInvoked();
         // restore the variable to its prior value
-        mf.AddDefinition(this->Args[0], oldDef);
+        mf.AddDefinition(this->Args.front(), oldDef);
         return true;
       }
       if (status.GetBreakInvoked()) {
         // restore the variable to its prior value
-        mf.AddDefinition(this->Args[0], oldDef);
+        mf.AddDefinition(this->Args.front(), oldDef);
         return true;
       }
       if (status.GetContinueInvoked()) {
@@ -102,11 +106,48 @@ bool cmForEachFunctionBlocker::Replay(
   }
 
   // restore the variable to its prior value
-  mf.AddDefinition(this->Args[0], oldDef);
+  mf.AddDefinition(this->Args.front(), oldDef);
   return true;
 }
+
+bool HandleInMode(std::vector<std::string> const& args, cmMakefile& makefile)
+{
+  auto fb = cm::make_unique<cmForEachFunctionBlocker>(&makefile);
+  fb->Args.push_back(args.front());
+
+  enum Doing
+  {
+    DoingNone,
+    DoingLists,
+    DoingItems
+  };
+  Doing doing = DoingNone;
+  for (std::string const& arg : cmMakeRange(args).advance(2)) {
+    if (arg == "LISTS") {
+      doing = DoingLists;
+    } else if (arg == "ITEMS") {
+      doing = DoingItems;
+    } else if (doing == DoingLists) {
+      auto const& value = makefile.GetSafeDefinition(arg);
+      if (!value.empty()) {
+        cmExpandList(value, fb->Args, true);
+      }
+    } else if (doing == DoingItems) {
+      fb->Args.push_back(arg);
+    } else {
+      makefile.IssueMessage(MessageType::FATAL_ERROR,
+                            cmStrCat("Unknown argument:\n", "  ", arg, "\n"));
+      return true;
+    }
+  }
+
+  makefile.AddFunctionBlocker(std::move(fb));
+
+  return true;
 }
 
+} // anonymous namespace
+
 bool cmForEachCommand(std::vector<std::string> const& args,
                       cmExecutionStatus& status)
 {
@@ -126,16 +167,16 @@ bool cmForEachCommand(std::vector<std::string> const& args,
       int stop = 0;
       int step = 0;
       if (args.size() == 3) {
-        stop = atoi(args[2].c_str());
+        stop = std::stoi(args[2]);
       }
       if (args.size() == 4) {
-        start = atoi(args[2].c_str());
-        stop = atoi(args[3].c_str());
+        start = std::stoi(args[2]);
+        stop = std::stoi(args[3]);
       }
       if (args.size() == 5) {
-        start = atoi(args[2].c_str());
-        stop = atoi(args[3].c_str());
-        step = atoi(args[4].c_str());
+        start = std::stoi(args[2]);
+        stop = std::stoi(args[3]);
+        step = std::stoi(args[4]);
       }
       if (step == 0) {
         if (start > stop) {
@@ -151,21 +192,24 @@ bool cmForEachCommand(std::vector<std::string> const& args,
                    ", stop ", stop, ", step ", step));
         return false;
       }
-      std::vector<std::string> range;
-      char buffer[100];
-      range.push_back(args[0]);
-      int cc;
-      for (cc = start;; cc += step) {
-        if ((step > 0 && cc > stop) || (step < 0 && cc < stop)) {
-          break;
-        }
-        sprintf(buffer, "%d", cc);
-        range.emplace_back(buffer);
-        if (cc == stop) {
-          break;
-        }
-      }
-      fb->Args = range;
+
+      // Calculate expected iterations count and reserve enough space
+      // in the `fb->Args` vector. The first item is the iteration variable
+      // name...
+      const std::size_t iter_cnt = 2u +
+        int(start < stop) * (stop - start) / std::abs(step) +
+        int(start > stop) * (start - stop) / std::abs(step);
+      fb->Args.resize(iter_cnt);
+      fb->Args.front() = args.front();
+      auto cc = start;
+      auto generator = [&cc, step]() -> std::string {
+        auto result = std::to_string(cc);
+        cc += step;
+        return result;
+      };
+      // Fill the `range` vector w/ generated string values
+      // (starting from 2nd position)
+      std::generate(++fb->Args.begin(), fb->Args.end(), generator);
     } else {
       fb->Args = args;
     }
@@ -176,42 +220,3 @@ bool cmForEachCommand(std::vector<std::string> const& args,
 
   return true;
 }
-
-namespace {
-bool HandleInMode(std::vector<std::string> const& args, cmMakefile& makefile)
-{
-  auto fb = cm::make_unique<cmForEachFunctionBlocker>(&makefile);
-  fb->Args.push_back(args[0]);
-
-  enum Doing
-  {
-    DoingNone,
-    DoingLists,
-    DoingItems
-  };
-  Doing doing = DoingNone;
-  for (unsigned int i = 2; i < args.size(); ++i) {
-    if (doing == DoingItems) {
-      fb->Args.push_back(args[i]);
-    } else if (args[i] == "LISTS") {
-      doing = DoingLists;
-    } else if (args[i] == "ITEMS") {
-      doing = DoingItems;
-    } else if (doing == DoingLists) {
-      const char* value = makefile.GetDefinition(args[i]);
-      if (value && *value) {
-        cmExpandList(value, fb->Args, true);
-      }
-    } else {
-      makefile.IssueMessage(
-        MessageType::FATAL_ERROR,
-        cmStrCat("Unknown argument:\n", "  ", args[i], "\n"));
-      return true;
-    }
-  }
-
-  makefile.AddFunctionBlocker(std::move(fb));
-
-  return true;
-}
-}

Tests/RunCMake/foreach/RunCMakeTest.cmake

@@ -1,3 +1,4 @@
 include(RunCMake)
 
 run_cmake(BadRangeInFunction)
+run_cmake(foreach-all-test)

Tests/RunCMake/foreach/foreach-all-test-stdout.txt

@@ -0,0 +1,44 @@
+-- foreach\(RANGE\):
+--   \[0\.\.1\]/1
+--     < 0
+--     < 1
+--   \[1\.\.1\]/1
+--     < 1
+--   \[0\.\.10\]/2
+--     < 0
+--     < 2
+--     < 4
+--     < 6
+--     < 8
+--     < 10
+--   \[-10\.\.0\]/3
+--     < -10
+--     < -7
+--     < -4
+--     < -1
+--   \[0\.\.-10\]/-5
+--     < 0
+--     < -5
+--     < -10
+-- foreach\(IN ITEMS\):
+--   < one
+--   < two
+--   < three
+-- foreach\(IN LISTS\):
+--   < satu
+--   < dua
+--   < tiga
+-- foreach\(IN LISTS and ITEMS\):
+--   < satu
+--   < dua
+--   < tiga
+--   < one
+--   < two
+--   < three
+-- foreach\(IN ITEMS and LISTS\):
+--   < one
+--   < two
+--   < three
+--   < satu
+--   < dua
+--   < tiga

Tests/RunCMake/foreach/foreach-all-test.cmake

@@ -0,0 +1,67 @@
+message(STATUS "foreach(RANGE):")
+list(APPEND CMAKE_MESSAGE_INDENT "  ")
+
+message(STATUS "[0..1]/1")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i RANGE 1)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "[1..1]/1")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i RANGE 1 1)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "[0..10]/2")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i RANGE 0 10 2)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "[-10..0]/3")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i RANGE -10 0 3)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "[0..-10]/-5")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i RANGE 0 -10 -5)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "foreach(IN ITEMS):")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i IN ITEMS one two three)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "foreach(IN LISTS):")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+list(APPEND count satu dua tiga)
+foreach(i IN LISTS count)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "foreach(IN LISTS and ITEMS):")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i IN LISTS count ITEMS one two three)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)
+
+message(STATUS "foreach(IN ITEMS and LISTS):")
+list(APPEND CMAKE_MESSAGE_INDENT "  < ")
+foreach(i IN ITEMS one two three LISTS count)
+    message(STATUS ${i})
+endforeach()
+list(POP_BACK CMAKE_MESSAGE_INDENT)

Utilities/IWYU/mapping.imp

@@ -24,6 +24,7 @@
   { include: [ "<bits/shared_ptr.h>", private, "<memory>", public ] },
   { include: [ "<bits/std_function.h>", private, "<functional>", public ] },
   { include: [ "<bits/refwrap.h>", private, "<functional>", public ] },
+  { include: [ "<bits/std_abs.h>", private, "<stdlib.h>", public ] },
   { include: [ "<bits/stdint-intn.h>", private, "<stdint.h>", public ] },
   { include: [ "<bits/stdint-uintn.h>", private, "<stdint.h>", public ] },
   { include: [ "<bits/time.h>", private, "<time.h>", public ] },