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

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 3 inline comments as done.
jhenderson added inline comments.


================
Comment at: unittests/Support/CommandLineTest.cpp:860-861
+
+  SmallVector<char, 128> FilePath;
+  bool Valid = true;
+
----------------
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.


================
Comment at: unittests/Support/CommandLineTest.cpp:917-918
+
+  EXPECT_EQ(Output, ("  -" + OptName + "=<value> - " + HelpText +
+                     "\n    =v1                -   desc1\n")
+                        .str());
----------------
thopre wrote:
> I understand the use of variable to avoid duplication but it makes it harder to see that we are testing for correct alignment. Could you choose variables with the right length to have the same alignment as the output text such that we can see the alignment by just looking at the text?
Sounds good to me. I'll do that.


================
Comment at: unittests/Support/CommandLineTest.cpp:929-930
+  EXPECT_EQ(Output,
+            ("  -" + OptName + "         - " + HelpText + "\n  -" + OptName +
+             "=<value> - " + HelpText + "\n    =v1                -   desc1\n")
+                .str());
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57030