[PATCH] D69501: [CommandLine] Add inline ArgName printing

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/lib/Support/CommandLine.cpp:91
 
-static StringRef ArgPrefix = "  -";
-static StringRef ArgPrefixLong = "  --";
+const unsigned DEFAULT_PAD = 2;
+
----------------
This should be `static`.


================
Comment at: llvm/lib/Support/CommandLine.cpp:106
+  SmallString<8> Prefix;
+  for (unsigned I = 0; I < Pad; ++I) {
+    Prefix.push_back(' ');
----------------
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.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1456
           *Errs << ProgramName << ": Did you mean '"
-                << PrintArg(NearestHandlerString) << "'?\n";
+                << PrintArg(NearestHandlerString, 0) << "'?\n";
         }
----------------
You should have a test for this change too, if feasible.


================
Comment at: llvm/unittests/Support/CommandLineTest.cpp:1658-1659
+TEST(CommandLineTest, OptionErrorMessage) {
+  // Test the fix for PR42943.
+  //
+  // When there is an error, we expect some error message like:
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69501/new/

https://reviews.llvm.org/D69501