Merge topic 'CMP0174-OLD-regression-repeated-keyword'

c8567acc32 cmake_parse_arguments: Restore capture of value after repeated keyword

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !9953

Help/policy/CMP0174.rst

@@ -10,21 +10,25 @@ One of the main reasons for using the ``PARSE_ARGV`` form of the
 :command:`cmake_parse_arguments` command is to more robustly handle corner
 cases related to empty values.  The non-``PARSE_ARGV`` form doesn't preserve
 empty arguments, but the ``PARSE_ARGV`` form does.  For each single-value
-keyword given, a variable should be defined if the keyword is followed by a
-value.  Prior to CMake 3.31, no variable would be defined if the value given
-was an empty string.  This meant the code could not detect the difference
-between the keyword not being given, and it being given but with an empty
-value, except by iterating over all the arguments and checking if the keyword
-is present.
+keyword given, a variable should be defined if the keyword is present, even
+if it is followed by an empty string.
+
+Prior to CMake 3.31, no variable would be defined if the value given after a
+single-value keyword was an empty string.  This meant the code could not detect
+the difference between the keyword not being given, and it being given but with
+an empty value, except by iterating over all the arguments and checking if the
+keyword is present.
 
 For the ``OLD`` behavior of this policy,
 :command:`cmake_parse_arguments(PARSE_ARGV)` does not define a variable for a
-single-value keyword followed by an empty string.
+single-value keyword followed by an empty string, or followed by no value at
+all.
+
 For the ``NEW`` behavior, :command:`cmake_parse_arguments(PARSE_ARGV)` always
 defines a variable for each keyword given in the arguments, even a single-value
-keyword with an empty string as its value.  With the ``NEW`` behavior, the
-code can robustly check if a single-value keyword was given with any value
-using just ``if(DEFINED <prefix>_<keyword>)``.
+keyword with an empty string as its value or no value at all.  With the
+``NEW`` behavior, the code can robustly check if a single-value keyword was
+given using just ``if(DEFINED <prefix>_<keyword>)``.
 
 .. |INTRODUCED_IN_CMAKE_VERSION| replace:: 3.31
 .. |WARNS_OR_DOES_NOT_WARN| replace:: warns

Source/cmParseArgumentsCommand.cxx

@@ -6,7 +6,6 @@
 #include <set>
 #include <utility>
 
-#include <cm/optional>
 #include <cm/string_view>
 
 #include "cmArgumentParser.h"
@@ -43,7 +42,7 @@ std::string JoinList(std::vector<std::string> const& arg, bool escape)
 }
 
 using options_map = std::map<std::string, bool>;
-using single_map = std::map<std::string, cm::optional<std::string>>;
+using single_map = std::map<std::string, std::string>;
 using multi_map =
   std::map<std::string, ArgumentParser::NonEmpty<std::vector<std::string>>>;
 using options_set = std::set<cm::string_view>;
@@ -76,7 +75,7 @@ struct UserArgumentParser : public cmArgumentParser<void>
 static void PassParsedArguments(
   const std::string& prefix, cmMakefile& makefile, const options_map& options,
   const single_map& singleValArgs, const multi_map& multiValArgs,
-  const std::vector<std::string>& unparsed,
+  const std::vector<std::string>& unparsed, const options_set& keywordsSeen,
   const options_set& keywordsMissingValues, bool parseFromArgV)
 {
   for (auto const& iter : options) {
@@ -87,29 +86,26 @@ static void PassParsedArguments(
   const cmPolicies::PolicyStatus cmp0174 =
     makefile.GetPolicyStatus(cmPolicies::CMP0174);
   for (auto const& iter : singleValArgs) {
-    if (iter.second.has_value()) {
-      // So far, we only know that the keyword was given, not whether a value
-      // was provided after the keyword
-      if (keywordsMissingValues.find(iter.first) ==
-          keywordsMissingValues.end()) {
-        // A (possibly empty) value was given
-        if (cmp0174 == cmPolicies::NEW || !iter.second->empty()) {
-          makefile.AddDefinition(cmStrCat(prefix, iter.first), *iter.second);
-          continue;
-        }
-        if (cmp0174 == cmPolicies::WARN) {
-          makefile.IssueMessage(
-            MessageType::AUTHOR_WARNING,
-            cmStrCat("An empty string was given as the value after the ",
-                     iter.first,
-                     " keyword. Policy CMP0174 is not set, so "
-                     "cmake_parse_arguments() will unset the ",
-                     prefix, iter.first,
-                     " variable rather than setting it to an empty string."));
-        }
+    if (keywordsSeen.find(iter.first) == keywordsSeen.end()) {
+      makefile.RemoveDefinition(cmStrCat(prefix, iter.first));
+    } else if ((parseFromArgV && cmp0174 == cmPolicies::NEW) ||
+               !iter.second.empty()) {
+      makefile.AddDefinition(cmStrCat(prefix, iter.first), iter.second);
+    } else {
+      // The OLD policy behavior doesn't define a variable for an empty or
+      // missing value, and we can't differentiate between those two cases.
+      if (parseFromArgV && (cmp0174 == cmPolicies::WARN)) {
+        makefile.IssueMessage(
+          MessageType::AUTHOR_WARNING,
+          cmStrCat("The ", iter.first,
+                   " keyword was followed by an empty string or no value at "
+                   "all. Policy CMP0174 is not set, so "
+                   "cmake_parse_arguments() will unset the ",
+                   prefix, iter.first,
+                   " variable rather than setting it to an empty string."));
       }
+      makefile.RemoveDefinition(cmStrCat(prefix, iter.first));
     }
-    makefile.RemoveDefinition(prefix + iter.first);
   }
 
   for (auto const& iter : multiValArgs) {
@@ -237,6 +233,14 @@ bool cmParseArgumentsCommand(std::vector<std::string> const& args,
     }
   }
 
+  std::vector<cm::string_view> keywordsSeen;
+  parser.BindParsedKeywords(keywordsSeen);
+
+  // For single-value keywords, only the last instance matters, since it
+  // determines the value stored. But if a keyword is repeated, it will be
+  // added to this vector if _any_ instance is missing a value. If one of the
+  // earlier instances is missing a value but the last one isn't, its presence
+  // in this vector will be misleading.
   std::vector<cm::string_view> keywordsMissingValues;
   parser.BindKeywordsMissingValue(keywordsMissingValues);
 
@@ -244,7 +248,7 @@ bool cmParseArgumentsCommand(std::vector<std::string> const& args,
 
   PassParsedArguments(
     prefix, status.GetMakefile(), options, singleValArgs, multiValArgs,
-    unparsed,
+    unparsed, options_set(keywordsSeen.begin(), keywordsSeen.end()),
     options_set(keywordsMissingValues.begin(), keywordsMissingValues.end()),
     parseFromArgV);
 

Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN-stderr.txt

@@ -2,7 +2,7 @@ Testing CMP0174 = NEW
 Testing CMP0174 = OLD
 Testing CMP0174 = WARN
 CMake Warning \(dev\) at CornerCasesArgvN\.cmake:[0-9]+ \(cmake_parse_arguments\):
-  An empty string was given as the value after the P1 keyword\.  Policy
+  The P1 keyword was followed by an empty string or no value at all\.  Policy
   CMP0174 is not set, so cmake_parse_arguments\(\) will unset the arg_P1
   variable rather than setting it to an empty string\.
 Call Stack \(most recent call first\):

Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN.cmake

@@ -60,7 +60,7 @@ block(SCOPE_FOR POLICIES)
     cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
     TEST(arg_KEYWORDS_MISSING_VALUES "P2")
     TEST(arg_P1 "")
-    TEST(arg_P2 "UNDEFINED")
+    TEST(arg_P2 "")
   endfunction()
   test_cmp0174_new_missing(P1 "" P2)
 
@@ -71,28 +71,56 @@ block(SCOPE_FOR POLICIES)
     TEST(arg_P2 "UNDEFINED")
   endfunction()
   test_cmp0174_new_no_missing(P1 "")
+
+  # For repeated keywords, the keyword will be reported as missing a value if
+  # any one of them is missing a value.
+  function(test_cmp0174_new_repeated_arg)
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    TEST(arg_KEYWORDS_MISSING_VALUES "P1")
+    TEST(arg_P1 "abc")
+    TEST(arg_P2 "UNDEFINED")
+  endfunction()
+  test_cmp0174_new_repeated_arg(P1 P1 abc)
 endblock()
 
 message(NOTICE "Testing CMP0174 = OLD")
 block(SCOPE_FOR POLICIES)
   cmake_policy(SET CMP0174 OLD)
   function(test_cmp0174_old)
-    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2;P3" "")
     TEST(arg_KEYWORDS_MISSING_VALUES "P2")
     TEST(arg_P1 "UNDEFINED")
     TEST(arg_P2 "UNDEFINED")
+    TEST(arg_P3 "UNDEFINED")
   endfunction()
   test_cmp0174_old(P1 "" P2)
+
+  function(test_cmp0174_old_repeated_arg)
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    TEST(arg_KEYWORDS_MISSING_VALUES "P1")
+    TEST(arg_P1 "abc")
+    TEST(arg_P2 "UNDEFINED")
+  endfunction()
+  test_cmp0174_old_repeated_arg(P1 P1 abc)
 endblock()
 
 message(NOTICE "Testing CMP0174 = WARN")
 block(SCOPE_FOR POLICIES)
   cmake_policy(VERSION 3.30)  # WARN
   function(test_cmp0174_warn)
-    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2;P3" "")
     TEST(arg_KEYWORDS_MISSING_VALUES "P2")
     TEST(arg_P1 "UNDEFINED")
     TEST(arg_P2 "UNDEFINED")
+    TEST(arg_P3 "UNDEFINED")
   endfunction()
   test_cmp0174_warn(P1 "" P2)
+
+  function(test_cmp0174_warn_repeated_arg)
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    TEST(arg_KEYWORDS_MISSING_VALUES "P1")
+    TEST(arg_P1 "abc")
+    TEST(arg_P2 "UNDEFINED")
+  endfunction()
+  test_cmp0174_warn_repeated_arg(P1 P1 abc)
 endblock()