[OptTable] Make new lines in help text respect indentation #74880
Conversation
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
|
@llvm/pr-subscribers-flang-driver @llvm/pr-subscribers-llvm-support Author: Andres Villegas (avillega) ChangesWith this changes, new lines in the HelpText defined Before, the help output will look something like: With this change: Note the "-" at the beginning of the help text in the new version, Full diff: https://github.com/llvm/llvm-project/pull/74880.diff 6 Files Affected:
diff --git a/llvm/lib/Option/OptTable.cpp b/llvm/lib/Option/OptTable.cpp
index 50eebf1f954bb..c69365540bed3 100644
--- a/llvm/lib/Option/OptTable.cpp
+++ b/llvm/lib/Option/OptTable.cpp
@@ -662,17 +662,27 @@ static void PrintHelpOptionList(raw_ostream &OS, StringRef Title,
}
const unsigned InitialPad = 2;
+ constexpr StringLiteral HelpSeparator("- ");
for (const OptionInfo &Opt : OptionHelp) {
const std::string &Option = Opt.Name;
- int Pad = OptionFieldWidth - int(Option.size());
+ int Pad = OptionFieldWidth + InitialPad;
+ int FirstLinePad = OptionFieldWidth - int(Option.size());
OS.indent(InitialPad) << Option;
// Break on long option names.
- if (Pad < 0) {
+ if (FirstLinePad < 0) {
OS << "\n";
- Pad = OptionFieldWidth + InitialPad;
+ FirstLinePad = OptionFieldWidth + InitialPad;
+ Pad = FirstLinePad;
}
- OS.indent(Pad + 1) << Opt.HelpText << '\n';
+
+ SmallVector<StringRef> Lines;
+ Opt.HelpText.split(Lines, '\n');
+ assert(Lines.size() && "Expected at least the first line in the help text");
+ auto *LinesIt = Lines.begin();
+ OS.indent(FirstLinePad + 1) << HelpSeparator << *LinesIt << '\n';
+ while (Lines.end() != ++LinesIt)
+ OS.indent(Pad + 1 + HelpSeparator.size()) << *LinesIt << '\n';
}
}
diff --git a/llvm/test/Support/check-default-options.txt b/llvm/test/Support/check-default-options.txt
index c71e608429da7..dc9d847a7199a 100644
--- a/llvm/test/Support/check-default-options.txt
+++ b/llvm/test/Support/check-default-options.txt
@@ -2,6 +2,6 @@
# RUN: llvm-tblgen --help-hidden %t | FileCheck --check-prefix=CHECK-TBLGEN %s
# RUN: llvm-opt-report --help-hidden %t | FileCheck --check-prefix=CHECK-OPT-RPT %s
-# CHECK-READOBJ: -h Alias for --file-header
+# CHECK-READOBJ: -h - Alias for --file-header
# CHECK-TBLGEN: -h - Alias for --help
# CHECK-OPT-RPT: -h - Alias for --help
diff --git a/llvm/test/tools/llvm-cvtres/help.test b/llvm/test/tools/llvm-cvtres/help.test
index f504e063ac152..3a29d80fcca1c 100644
--- a/llvm/test/tools/llvm-cvtres/help.test
+++ b/llvm/test/tools/llvm-cvtres/help.test
@@ -4,13 +4,13 @@
; HELP_TEST: OVERVIEW: Resource Converter
; HELP_TEST-DAG: USAGE: llvm-cvtres [options] file...
; HELP_TEST-DAG: OPTIONS:
-; HELP_TEST-NEXT: /{{DEFINE}}:symbol Not implemented
-; HELP_TEST-NEXT: /FOLDDUPS: Not implemented
-; HELP_TEST-NEXT: /HELP Display available options
+; HELP_TEST-NEXT: /{{DEFINE}}:symbol - Not implemented
+; HELP_TEST-NEXT: /FOLDDUPS: - Not implemented
+; HELP_TEST-NEXT: /HELP - Display available options
; HELP_TEST-NEXT: /MACHINE:{ARM|ARM64|EBC|IA64|X64|X86}
-; HELP_TEST-NEXT: Machine architecture
-; HELP_TEST-DAG: /NOLOGO Not implemented
-; HELP_TEST-NEXT: /OUT:filename Output file
-; HELP_TEST-NEXT: /READONLY Not implemented
-; HELP_TEST-NEXT: /TIMESTAMP:<value> Timestamp for coff header, defaults to current time
-; HELP_TEST-NEXT: /VERBOSE Use verbose output
+; HELP_TEST-NEXT: - Machine architecture
+; HELP_TEST-DAG: /NOLOGO - Not implemented
+; HELP_TEST-NEXT: /OUT:filename - Output file
+; HELP_TEST-NEXT: /READONLY - Not implemented
+; HELP_TEST-NEXT: /TIMESTAMP:<value> - Timestamp for coff header, defaults to current time
+; HELP_TEST-NEXT: /VERBOSE - Use verbose output
diff --git a/llvm/test/tools/llvm-rc/helpmsg.test b/llvm/test/tools/llvm-rc/helpmsg.test
index ecda426e3280a..f312882051dc1 100644
--- a/llvm/test/tools/llvm-rc/helpmsg.test
+++ b/llvm/test/tools/llvm-rc/helpmsg.test
@@ -6,18 +6,18 @@
; CHECK: OVERVIEW: LLVM Resource Converter
; CHECK-DAG: USAGE: llvm-rc [options] file...
; CHECK-DAG: OPTIONS:
-; CHECK-NEXT: /? Display this help and exit.
-; CHECK-NEXT: /C <value> Set the codepage used for input strings.
-; CHECK-NEXT: /dry-run Don't compile the input; only try to parse it.
-; CHECK-NEXT: /D <value> Define a symbol for the C preprocessor.
-; CHECK-NEXT: /FO <value> Change the output file location.
-; CHECK-NEXT: /H Display this help and exit.
-; CHECK-NEXT: /I <value> Add an include path.
-; CHECK-NEXT: /LN <value> Set the default language name.
-; CHECK-NEXT: /L <value> Set the default language identifier.
-; CHECK-NEXT: /no-preprocess Don't try to preprocess the input file.
-; CHECK-NEXT: /N Null-terminate all strings in the string table.
-; CHECK-NEXT: /U <value> Undefine a symbol for the C preprocessor.
-; CHECK-NEXT: /V Be verbose.
-; CHECK-NEXT: /X Ignore 'include' variable.
-; CHECK-NEXT: /Y Suppress warnings on duplicate resource IDs.
+; CHECK-NEXT: /? - Display this help and exit.
+; CHECK-NEXT: /C <value> - Set the codepage used for input strings.
+; CHECK-NEXT: /dry-run - Don't compile the input; only try to parse it.
+; CHECK-NEXT: /D <value> - Define a symbol for the C preprocessor.
+; CHECK-NEXT: /FO <value> - Change the output file location.
+; CHECK-NEXT: /H - Display this help and exit.
+; CHECK-NEXT: /I <value> - Add an include path.
+; CHECK-NEXT: /LN <value> - Set the default language name.
+; CHECK-NEXT: /L <value> - Set the default language identifier.
+; CHECK-NEXT: /no-preprocess - Don't try to preprocess the input file.
+; CHECK-NEXT: /N - Null-terminate all strings in the string table.
+; CHECK-NEXT: /U <value> - Undefine a symbol for the C preprocessor.
+; CHECK-NEXT: /V - Be verbose.
+; CHECK-NEXT: /X - Ignore 'include' variable.
+; CHECK-NEXT: /Y - Suppress warnings on duplicate resource IDs.
diff --git a/llvm/test/tools/llvm-readobj/basic.test b/llvm/test/tools/llvm-readobj/basic.test
index 73912373d7d2f..8887f0641435c 100644
--- a/llvm/test/tools/llvm-readobj/basic.test
+++ b/llvm/test/tools/llvm-readobj/basic.test
@@ -54,8 +54,8 @@ HELP: OVERVIEW: LLVM Object Reader
OBJ: llvm-readobj{{.*}} [options] <input object files>
ELF: llvm-readelf{{.*}} [options] <input object files>
HELP: OPTIONS:
-HELP: -s Alias for --symbols
-HELP: -t Alias for --section-details
+HELP: -s - Alias for --symbols
+HELP: -t - Alias for --section-details
HELP: OPTIONS (ELF specific):
HELP: --dynamic-table
HELP: OPTIONS (Mach-O specific):
diff --git a/llvm/test/tools/llvm-readtapi/command-line.test b/llvm/test/tools/llvm-readtapi/command-line.test
index 1006bfe9cc253..e8881348253c4 100644
--- a/llvm/test/tools/llvm-readtapi/command-line.test
+++ b/llvm/test/tools/llvm-readtapi/command-line.test
@@ -8,7 +8,7 @@
CHECK: OVERVIEW: LLVM TAPI file reader and manipulator
CHECK: USAGE: llvm-readtapi [options] [-arch <arch>]* <inputs> [-o <output>]*
CHECK: OPTIONS:
-CHECK: -help display this help
+CHECK: -help - display this help
MULTI_ACTION: error: only one of the following actions can be specified: -merge -compare
FILE_FORMAT: error: deprecated filetype 'tbd-v2' is not supported to write
|
|
Thanks. This is an improvement. Some tests may need Sometimes we switch to the following check prefix style: |
I don't fully understand your comment, I am working on getting some of the test to be whitespace strict to specifically test the functionality. |
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
Created using spr 1.3.4
llvmbot
added
flang:driver
flang
There was a problem hiding this comment.
Is the main functionality of this test actually tested in the current patch? It doesn't seem to be to me.
I don't fully understand your comment, I am working on getting some of the test to be whitespace strict to specifically test the functionality.
I just didn't see any testing like you've now added in OptionParsingTest.cpp that showed that the indentation was working correctly as desired.
Taking a step back, I'm not sure the addition of the dash has anything to do with the main purpose of this PR, and I'm a little worried people will not spot that this PR is adding it. I personally have no issue with that part of the change, but I wonder if others wouldn't want it?
| ; HELP_TEST: OVERVIEW: Resource Converter | ||
| ; HELP_TEST-DAG: USAGE: llvm-cvtres [options] file... | ||
| ; HELP_TEST-DAG: OPTIONS: | ||
| ; HELP_TEST-NEXT: /{{DEFINE}}:symbol - Not implemented |
There was a problem hiding this comment.
The formatting in this line looks wrong? It looks to me like there might be a bug here (possibly predating your PR).
There was a problem hiding this comment.
This is a quirk of FileCheck. It thinks that DEFINE is a directive, so in order to match the DEFINE word it is necessary to make it a regex. FileCheck will ignore {{}} and that is why it doesn't look at the same indentation level as the rest of the options, but it is.
avillega
changed the base branch from
users/avillega/main.opttable-make-new-lines-in-help-text-respect-indentation
to
main
avillega
requested review from
antiagainst,
kuhar,
hanhanW,
nicolasvasilache,
MaheshRavishankar,
dcaballe,
banach-space,
JDevlieghere,
Endilll and
nikic
as code owners
avillega
changed the base branch from
main
to
users/avillega/main.opttable-make-new-lines-in-help-text-respect-indentation
avillega
removed request for
a team,
nikic,
antiagainst,
JDevlieghere,
MaheshRavishankar,
banach-space,
kuhar,
hanhanW,
nicolasvasilache,
Endilll and
dcaballe
I think |
I will remove it. But I added it for two reasons:
|
Created using spr 1.3.5 [skip ci]
Created using spr 1.3.5
avillega
changed the base branch from
users/avillega/main.opttable-make-new-lines-in-help-text-respect-indentation
to
main
avillega
changed the base branch from
main
to
users/avillega/main.opttable-make-new-lines-in-help-text-respect-indentation
avillega
changed the base branch from
users/avillega/main.opttable-make-new-lines-in-help-text-respect-indentation
to
main
Created using spr 1.3.5
avillega
changed the base branch from
main
to
users/avillega/main.opttable-make-new-lines-in-help-text-respect-indentation-1
There was a problem hiding this comment.
I don't know if this is due to your use of spr or something, but the rebasing has made this change pretty much impossible to review, because it has introduced a whole load of changes that have nothing to do with the original patch, and there's no longer a convenient commit at the end of the stack that I can review.
The github workflow is a pain, and spr is not making it any better, at least for this specific PR. I created another PR with the same changes and added you as reviewers. New PR at: #75366 |
avillega
deleted the
users/avillega/opttable-make-new-lines-in-help-text-respect-indentation
branch
It seems spr that a few contributors are using does solve my complaint about the merge-upstream workflow (https://maskray.me/blog/2023-09-09-reflections-on-llvm-switch-to-github-pull-requests#patch-evolution) It seems that reviewers need to learn to ignore these spr-generated rebase commits. |
I tried clicking on the option to review the specific commit that actually contained changes, but it also always selected the immediately-previous spr commit that included a rebase, and as such it became impossible to review individually. More generally, it's no longer possible to review just the actual changes as a whole (ignoring things introduced via rebase), as far as I can tell. |
With this changes, new lines in the HelpText defined
in OptTable have the same indentation as the first line.
Before, the help output will look something like:
With this change:
Note the "-" at the beginning of the help text in the new version,
it was added to clearly separate the HelpText of the different
options.