[CommandLine] Add inline ArgName printing

[dsprenkels]

Summary

This patch adds PrintArgInline (after PrintArg) that strips the
leading spaces from an argument before printing them, for usage
inline.

Related bug: PR42943 https://bugs.llvm.org/show_bug.cgi?id=42943

[jhenderson]

Hi @dsprenkels,

Thanks for the patch. I've just got back from a couple of weeks off, and have a lot to catch up on, but I'll get back to reviewing this in due course (it may be a few days, depending on how things go). In the meantime, you might want to look at who else has recently modified this file and add them as reviewers too.

You also should add one or more tests to show that the bug this issue fixes has been fixed.

[dsprenkels]

Added OptionErrorMessage test case in CommandLineTest for PR42943.

[dsprenkels]

@jhenderson, I added the test.

The only person that has modified llvm/lib/Support/CommandLine.cpp a couple of times in the last year is @hintonda. I added them as a reviewer.

[hintonda]

In D69501#1723723, @dsprenkels wrote:

@jhenderson, I added the test.

The only person that has modified llvm/lib/Support/CommandLine.cpp a couple of times in the last year is @hintonda. I added them as a reviewer.

Thanks for the patch.

I originally added this to cleanup printing help, so I appreciate you fixing the error messages too.

llvm/lib/Support/CommandLine.cpp

136

Instead of adding a new class that's only used 3 times, please consider passing an additional prefix padding variable that defaults to the current behavior, 2, and then pass 0 for the three error messages you fixed.

That way, you can forward the padding down to argPrefix() and clean it up while you're at it -- it could use a bit of refactoring too.

[dsprenkels]

Updated the diff, incorporating @hintonda's suggestions.

[jhenderson]

I think adding an extra constructor argument to PrintArg might be the way forward. There are several places that add additional spaces before the PrintArg call (see e.g. printGenericOptionDiff and printOptionInfo in generic_parser_base), where the spaces could be combined with the two implicitly added currently, I think by specifying, e.g. "4" as the extra argument. The alternative is to go the other way and remove the spaces that PrintArg automatically adds, and then add them in the relevant places explicitly. I'm not sure how clean that would be though.

[jhenderson]

Not that it matters, but something odd has happened with your context: the CommandLineTest.cpp file is missing all context after your change. I suspect it's the end of the file, but it might mean you've got a mistake in whatever you are using to create the diff.

llvm/lib/Support/CommandLine.cpp

92

This should be static.

107

Nit: size_t might be more appropriate, since you're dealing with the size of a data structure here. Probably the same goes for the Pad variables.

1463

You should have a test for this change too, if feasible.

llvm/unittests/Support/CommandLineTest.cpp

1658–1659

I'd delete this reference to the bugzilla. It'll go stale very quickly, especially as we might well move to Github issues very soon, and doesn't really add anything.

[dsprenkels]

Updated the diff incorporating the new comments.

[dsprenkels]

Updated the diff to one generated in the correct way.

[dsprenkels]

@jhenderson, you were right. I forgot how to upload patches correctly, and that is how the context got lost. The new diff (226891) should be exported correctly. Thanks for the heads-up.

[jhenderson]

Aside from one formatting nit, this looks good to me. Please let @hintonda provide any more comments though before this gets committed.

llvm/lib/Support/CommandLine.cpp

1463

As an aside, once you've addressed a review comment, it's helpful if you select the "Done" button for each of those comments. You can do this prior to uploading the diff and they will get submitted when you upload too.

llvm/unittests/Support/CommandLineTest.cpp

1700

Did you remember to run clang-format?

[dsprenkels]

Fixed formatting using clang-format.

As an aside, once you've addressed a review comment, it's helpful if you select the "Done" button for each of those comments. You can do this prior to uploading the diff and they will get submitted when you upload too.

As you might have noticed, this is my first review on Phabricator. Thanks for your patience. :)

[hintonda]

LGTM, thanks for the patch!

[hintonda]

I see you haven't landed this yet. Do you have commit rights? If not, I'd be happy to land it for you.

[dsprenkels]

I don't have commit right. This is one of my first patches. Can you do it for me please?

llvm/lib/Support/CommandLine.cpp

 void parser<double>::anchor() {}
 void parser<float>::anchor() {}
 void parser<std::string>::anchor() {}
 void parser<char>::anchor() {}
 
 //===----------------------------------------------------------------------===//
 
 static StringRef ArgPrefix = "  -";
 static StringRef ArgPrefixLong = "  --";
 static StringRef ArgHelpPrefix = " - ";
 
 static size_t argPlusPrefixesSize(StringRef ArgName) {
   size_t Len = ArgName.size();
   if (Len == 1)
     return Len + ArgPrefix.size() + ArgHelpPrefix.size();
   return Len + ArgPrefixLong.size() + ArgHelpPrefix.size();
 }
 
 static StringRef argPrefix(StringRef ArgName) {
   if (ArgName.size() == 1)
     return ArgPrefix;
   return ArgPrefixLong;
 }
 
 // Option predicates...
 static inline bool isGrouping(const Option *O) {
   return O->getMiscFlags() & cl::Grouping;
 }
 static inline bool isPrefixedOrGrouping(const Option *O) {
   return isGrouping(O) || O->getFormattingFlag() == cl::Prefix ||
          O->getFormattingFlag() == cl::AlwaysPrefix;
 }
 
 
 namespace {
 
 class PrintArg {
   StringRef ArgName;
 public:
   PrintArg(StringRef ArgName) : ArgName(ArgName) {}
   friend raw_ostream &operator<<(raw_ostream &OS, const PrintArg&);
 };
 
 raw_ostream &operator<<(raw_ostream &OS, const PrintArg& Arg) {
   OS << argPrefix(Arg.ArgName) << Arg.ArgName;
   return OS;
 }
 
+// Use for inline printing (omit leading spaces)
+class PrintArgInline {
+  StringRef ArgName;
+public:
+  PrintArgInline(StringRef ArgName) : ArgName(ArgName) {}
+  friend raw_ostream &operator<<(raw_ostream &OS, const PrintArgInline &);
+};
+
+raw_ostream &operator<<(raw_ostream &OS, const PrintArgInline &Arg) {
+  OS << argPrefix(Arg.ArgName).ltrim() << Arg.ArgName;
+  return OS;
+}
+
 class CommandLineParser {
 public:
   // Globals for name and overview of program.  Program name is not a string to
   // avoid static ctor/dtor issues.
   std::string ProgramName;
   StringRef ProgramOverview;
 
   // This collects additional help to be printed.
for (int i = FirstArg; i < argc; ++i) {
     if (!Handler) {
       if (SinkOpts.empty()) {
         *Errs << ProgramName << ": Unknown command line argument '" << argv[i]
               << "'.  Try: '" << argv[0] << " --help'\n";
 
         if (NearestHandler) {
           // If we know a near match, report it as well.
           *Errs << ProgramName << ": Did you mean '"
-                << PrintArg(NearestHandlerString) << "'?\n";
+                << PrintArgInline(NearestHandlerString) << "'?\n";
         }
 
         ErrorParsing = true;
       } else {
         for (SmallVectorImpl<Option *>::iterator I = SinkOpts.begin(),
                                                  E = SinkOpts.end();
              I != E; ++I)
           (*I)->addOccurrence(i, "", StringRef(argv[i]));

 //
 
 bool Option::error(const Twine &Message, StringRef ArgName, raw_ostream &Errs) {
   if (!ArgName.data())
     ArgName = ArgStr;
   if (ArgName.empty())
     Errs << HelpStr; // Be nice for positional arguments
   else
-    Errs << GlobalParser->ProgramName << ": for the " << PrintArg(ArgName);
+    Errs << GlobalParser->ProgramName << ": for the " << PrintArgInline(ArgName);
 
   Errs << " option: " << Message << "\n";
   return true;
 }
 
 bool Option::addOccurrence(unsigned pos, StringRef ArgName, StringRef Value,
                            bool MultiArg) {
   if (!MultiArg)

llvm/unittests/Support/CommandLineTest.cpp

EXPECT_TRUE(cl::ParseCommandLineOptions(4, args3, StringRef(),
                                            &OS, nullptr, true));
   EXPECT_TRUE(OptA);
   EXPECT_TRUE(OptBLong);
   EXPECT_STREQ("val1", OptAB.c_str());
   OS.flush();
   EXPECT_TRUE(Errs.empty()); Errs.clear();
   cl::ResetAllOptionOccurrences();
 }
+
+TEST(CommandLineTest, OptionErrorMessage) {
+  // Test the fix for PR42943.
+  //
+  // When there is an error, we expect some error message like:
+  //   prog: for the -a option: [...]
+  //
+  // Test whether the "for the -a option"-part is correctly formatted.
+  cl::ResetCommandLineParser();
+
+  StackOption<bool> OptA("a", cl::desc("Some option"));
+  StackOption<bool> OptLong("long", cl::desc("Some long option"));
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+
+  OptA.error("custom error", OS);
+  OS.flush();
+  EXPECT_FALSE(Errs.find("for the -a option:") == std::string::npos);
+  Errs.clear();
+
+  OptLong.error("custom error", OS);
+  OS.flush();
+  EXPECT_FALSE(Errs.find("for the --long option:") == std::string::npos);
+  Errs.clear();
+
+  cl::ResetAllOptionOccurrences();
+}
 }  // anonymous namespace