Don't print a comma in a two-item list. (#4596)

common/command_line.cpp

@@ -55,6 +55,21 @@ auto operator<<(llvm::raw_ostream& output, CommandKind kind)
   }
   CARBON_FATAL("Corrupt command kind!");
 }
+
+template <typename T, typename ToPrintable>
+static auto PrintListOfAlternatives(llvm::raw_ostream& output,
+                                    llvm::ArrayRef<T> alternatives,
+                                    ToPrintable to_printable) -> void {
+  for (const auto& alternative : alternatives.drop_back()) {
+    output << "`" << to_printable(alternative)
+           << (alternatives.size() > 2 ? "`, " : "` ");
+  }
+  if (alternatives.size() > 1) {
+    output << "or ";
+  }
+  output << "`" << to_printable(alternatives.back()) << "`";
+}
+
 Arg::Arg(ArgInfo info) : info(info) {}
 
 Arg::~Arg() {
@@ -328,14 +343,10 @@ void MetaPrinter::PrintVersion(const Command& command) const {
 }
 
 void MetaPrinter::PrintSubcommands(const Command& command) const {
-  for (const auto& subcommand :
-       llvm::ArrayRef(command.subcommands).drop_back()) {
-    *out_ << "`" << subcommand->info.name << "`, ";
-  }
-  if (command.subcommands.size() > 1) {
-    *out_ << "or ";
-  }
-  *out_ << "`" << command.subcommands.back()->info.name << "`";
+  PrintListOfAlternatives(*out_, llvm::ArrayRef(command.subcommands),
+                          [](const std::unique_ptr<Command>& subcommand) {
+                            return subcommand->info.name;
+                          });
 }
 
 void MetaPrinter::PrintRawVersion(const Command& command,
@@ -903,13 +914,8 @@ auto Parser::ParseOneOfArgValue(const Arg& arg, llvm::StringRef value)
     error << "` has an invalid value `";
     llvm::printEscapedString(value, error);
     error << "`; valid values are: ";
-    for (auto value_string : arg.value_strings.drop_back()) {
-      error << "`" << value_string << "`, ";
-    }
-    if (arg.value_strings.size() > 1) {
-      error << "or ";
-    }
-    error << "`" << arg.value_strings.back() << "`";
+    PrintListOfAlternatives(error, arg.value_strings,
+                            [](llvm::StringRef x) { return x; });
     return Error(error_str);
   }
   return Success();

common/command_line_test.cpp

@@ -361,7 +361,7 @@ TEST(ArgParserTest, BasicSubcommands) {
 
   EXPECT_THAT(parse({"sub2", "--option=abc", "subsub42", "--option=xyz"}, os),
               IsError(StrEq("invalid subcommand `subsub42`; available "
-                            "subcommands: `subsub`, or `help`")));
+                            "subcommands: `subsub` or `help`")));
 
   EXPECT_THAT(
       parse({"sub2", "--option=abc", "subsub", "--option=xyz"}, llvm::errs()),
@@ -434,20 +434,27 @@ TEST(ArgParserTest, PositionalAppending) {
   EXPECT_THAT(strings3, ElementsAre(StrEq("e"), StrEq("f")));
 }
 
+static auto ParseOneOfOption(llvm::ArrayRef<llvm::StringRef> args,
+                             llvm::raw_ostream& s,
+                             llvm::function_ref<void(OneOfArgBuilder&)> build)
+    -> ErrorOr<ParseResult> {
+  return Parse(args, s, TestCommandInfo, [&](auto& b) {
+    b.AddOneOfOption({.name = "option"}, build);
+    b.Do([] {});
+  });
+}
+
 TEST(ArgParserTest, OneOfOption) {
   int value = 0;
   auto parse = [&](llvm::ArrayRef<llvm::StringRef> args, llvm::raw_ostream& s) {
-    return Parse(args, s, TestCommandInfo, [&](auto& b) {
-      b.AddOneOfOption({.name = "option"}, [&](auto& arg_b) {
-        arg_b.SetOneOf(
-            {
-                arg_b.OneOfValue("x", 1),
-                arg_b.OneOfValue("y", 2),
-                arg_b.OneOfValue("z", 3),
-            },
-            &value);
-      });
-      b.Do([] {});
+    return ParseOneOfOption(args, s, [&](auto& arg_b) {
+      arg_b.SetOneOf(
+          {
+              arg_b.OneOfValue("x", 1),
+              arg_b.OneOfValue("y", 2),
+              arg_b.OneOfValue("z", 3),
+          },
+          &value);
     });
   };
 
@@ -489,20 +496,48 @@ TEST(ArgParserTest, OneOfOption) {
               IsError(StrEq(llvm::formatv(ErrorStr, "\\00"))));
 }
 
+TEST(ArgParserTest, OneOfOptionWithTwoOptions) {
+  int value = 0;
+  TestRawOstream os;
+  EXPECT_THAT(ParseOneOfOption({"--option=z"}, os,
+                               [&](auto& arg_b) {
+                                 arg_b.SetOneOf(
+                                     {
+                                         arg_b.OneOfValue("x", 1),
+                                         arg_b.OneOfValue("y", 2),
+                                     },
+                                     &value);
+                               }),
+              IsError(StrEq("option `--option=z` has an invalid value `z`; "
+                            "valid values are: `x` or `y`")));
+}
+
+TEST(ArgParserTest, OneOfOptionWithOneOption) {
+  int value = 0;
+  TestRawOstream os;
+  EXPECT_THAT(ParseOneOfOption({"--option=z"}, os,
+                               [&](auto& arg_b) {
+                                 arg_b.SetOneOf(
+                                     {
+                                         arg_b.OneOfValue("x", 1),
+                                     },
+                                     &value);
+                               }),
+              IsError(StrEq("option `--option=z` has an invalid value `z`; "
+                            "valid values are: `x`")));
+}
+
 TEST(ArgParserTest, OneOfOptionAppending) {
   llvm::SmallVector<int> values;
   auto parse = [&](llvm::ArrayRef<llvm::StringRef> args, llvm::raw_ostream& s) {
-    return Parse(args, s, TestCommandInfo, [&](auto& b) {
-      b.AddOneOfOption({.name = "option"}, [&](auto& arg_b) {
-        arg_b.AppendOneOf(
-            {
-                arg_b.OneOfValue("x", 1),
-                arg_b.OneOfValue("y", 2),
-                arg_b.OneOfValue("z", 3),
-            },
-            &values);
-      });
-      b.Do([] {});
+    return ParseOneOfOption(args, s, [&](auto& arg_b) {
+      arg_b.AppendOneOf(
+          {
+              arg_b.OneOfValue("x", 1),
+              arg_b.OneOfValue("y", 2),
+              arg_b.OneOfValue("z", 3),
+          },
+          &values);
     });
   };