[PATCH] D57030: [CommandLine] Don't print empty sentinel values from EnumValN lists in help text

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: unittests/Support/CommandLineTest.cpp:860-861
+
+  SmallVector<char, 128> FilePath;
+  bool Valid = true;
+
----------------
jhenderson wrote:
> thopre wrote:
> > Why not make them private with getters? Did you deem it over-engineering?
> Yes, essentially. If you prefer me to add getters, I don't mind.
I'm thinking more in terms of readability as someone reading understands from getter without setter that it's a constant value. I would have prefer a simple const though, and the testcase is small enough and obvious enough that it's not much of a problem. Maybe you made the good call, let's leave it as is.


================
Comment at: unittests/Support/CommandLineTest.cpp:929-930
+  EXPECT_EQ(Output,
+            ("  -" + OptName + "         - " + HelpText + "\n  -" + OptName +
+             "=<value> - " + HelpText + "\n    =v1                -   desc1\n")
+                .str());
----------------
jhenderson wrote:
> thopre wrote:
> > I think it's better to go to next line for every \n, I'd suggest also keeping the \n at end of line to make the reference string as close as the output. Likewise for other tests below and above.
> That's what I wanted to do, but this is how clang-format lines it up. I'm happy to not run clang-format on this section, if you are.
I'm quite new to the community so not sure how is it viewed to ignore clang-format. In my opinion formatting is to improve readability by consistency, in this case having things as close as possible to the output is more readable so I'd be in favour of ignoring clang-format just for that bit yes.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57030