[lldb] Underline short option letters as mnemonics #153695
Conversation
Whenever an option would use something other than the first letter of the long option as the short option, Jim would capitalized the letter we picked as a mnemonic. This has often been mistaken for a typo and Jim wondered if we should stop doing this. During the discussion, David mentioned how this reminds him of the underline in menu bars when holding down alt. I suggested we do something similar in LLDB by underlying the letter in the description. See https://discourse.llvm.org/t/should-we-remove-the-capital-letter-in-option-helps/87816
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesWhenever an option would use something other than the first letter of the long option as the short option, Jim would capitalized the letter we picked as a mnemonic. This has often been mistaken for a typo and Jim wondered if we should stop doing this. During the discussion, David mentioned how this reminds him of the underline in menu bars when holding down alt. I suggested we do something similar in LLDB by underlying the letter in the description. Here's what that looks like for a subset of the <img width="570" height="165" alt="Screenshot 2025-08-14 at 4 49 12 PM" src="https://github.com/user-attachments/assets/576caade-11d5-4595-9676-46d012257d42" /> See https://discourse.llvm.org/t/should-we-remove-the-capital-letter-in-option-helps/87816 Full diff: https://github.com/llvm/llvm-project/pull/153695.diff 7 Files Affected:
diff --git a/lldb/include/lldb/Interpreter/Options.h b/lldb/include/lldb/Interpreter/Options.h
index 864bda6f24c8c..2d06605f5f0b0 100644
--- a/lldb/include/lldb/Interpreter/Options.h
+++ b/lldb/include/lldb/Interpreter/Options.h
@@ -85,10 +85,10 @@ class Options {
void OutputFormattedUsageText(Stream &strm,
const OptionDefinition &option_def,
- uint32_t output_max_columns);
+ uint32_t output_max_columns, bool use_color);
void GenerateOptionUsage(Stream &strm, CommandObject &cmd,
- uint32_t screen_width);
+ uint32_t screen_width, bool use_color);
bool SupportsLongOption(const char *long_option);
diff --git a/lldb/source/Commands/CommandObjectDisassemble.cpp b/lldb/source/Commands/CommandObjectDisassemble.cpp
index 70e687e19ac6d..f8e4ff9f08b6c 100644
--- a/lldb/source/Commands/CommandObjectDisassemble.cpp
+++ b/lldb/source/Commands/CommandObjectDisassemble.cpp
@@ -503,8 +503,9 @@ void CommandObjectDisassemble::DoExecute(Args &command,
"\"disassemble\" arguments are specified as options.\n");
const int terminal_width =
GetCommandInterpreter().GetDebugger().GetTerminalWidth();
+ const bool use_color = GetCommandInterpreter().GetDebugger().GetUseColor();
GetOptions()->GenerateOptionUsage(result.GetErrorStream(), *this,
- terminal_width);
+ terminal_width, use_color);
return;
}
diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index 56926999d1678..7e58a95fae2c3 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -349,7 +349,8 @@ class CommandObjectFrameSelect : public CommandObjectParsed {
command[0].c_str());
m_options.GenerateOptionUsage(
result.GetErrorStream(), *this,
- GetCommandInterpreter().GetDebugger().GetTerminalWidth());
+ GetCommandInterpreter().GetDebugger().GetTerminalWidth(),
+ GetCommandInterpreter().GetDebugger().GetUseColor());
return;
}
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index dbebbbd38093e..3ae08dec75e31 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -4075,7 +4075,8 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed {
default:
m_options.GenerateOptionUsage(
result.GetErrorStream(), *this,
- GetCommandInterpreter().GetDebugger().GetTerminalWidth());
+ GetCommandInterpreter().GetDebugger().GetTerminalWidth(),
+ GetCommandInterpreter().GetDebugger().GetUseColor());
syntax_error = true;
break;
}
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index 61acc40585976..ac2817d158a05 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -736,18 +736,35 @@ let Command = "process launch" in {
}
let Command = "process attach" in {
- def process_attach_continue : Option<"continue", "c">,
- Desc<"Immediately continue the process once attached.">;
- def process_attach_plugin : Option<"plugin", "P">, Arg<"Plugin">,
- Desc<"Name of the process plugin you want to use.">;
- def process_attach_pid : Option<"pid", "p">, Group<1>, Arg<"Pid">,
- Desc<"The process ID of an existing process to attach to.">;
- def process_attach_name : Option<"name", "n">, Group<2>, Arg<"ProcessName">,
- Desc<"The name of the process to attach to.">;
- def process_attach_include_existing : Option<"include-existing", "i">,
- Group<2>, Desc<"Include existing processes when doing attach -w.">;
- def process_attach_waitfor : Option<"waitfor", "w">, Group<2>,
- Desc<"Wait for the process with <process-name> to launch.">;
+ def process_attach_continue
+ : Option<"continue", "c">,
+ Desc<"Immediately ${ansi.underline}c${ansi.normal}ontinue the process "
+ "once attached.">;
+ def process_attach_plugin
+ : Option<"plugin", "P">,
+ Arg<"Plugin">,
+ Desc<"Name of the process ${ansi.underline}p${ansi.normal}lugin you "
+ "want to use.">;
+ def process_attach_pid : Option<"pid", "p">,
+ Group<1>,
+ Arg<"Pid">,
+ Desc<"The ${ansi.underline}p${ansi.normal}rocess ID "
+ "of an existing process to attach to.">;
+ def process_attach_name : Option<"name", "n">,
+ Group<2>,
+ Arg<"ProcessName">,
+ Desc<"The ${ansi.underline}n${ansi.normal}ame of "
+ "the process to attach to.">;
+ def process_attach_include_existing
+ : Option<"include-existing", "i">,
+ Group<2>,
+ Desc<"${ansi.underline}I${ansi.normal}nclude existing processes when "
+ "doing attach -w.">;
+ def process_attach_waitfor
+ : Option<"waitfor", "w">,
+ Group<2>,
+ Desc<"${ansi.underline}W${ansi.normal}ait for the process with "
+ "<process-name> to launch.">;
}
let Command = "process continue" in {
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 129646ebddb94..43e19b397ae1f 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -359,7 +359,8 @@ bool CommandObject::HelpTextContainsWord(llvm::StringRef search_word,
StreamString usage_help;
GetOptions()->GenerateOptionUsage(
usage_help, *this,
- GetCommandInterpreter().GetDebugger().GetTerminalWidth());
+ GetCommandInterpreter().GetDebugger().GetTerminalWidth(),
+ GetCommandInterpreter().GetDebugger().GetUseColor());
if (!usage_help.Empty()) {
llvm::StringRef usage_text = usage_help.GetString();
if (usage_text.contains_insensitive(search_word))
@@ -672,7 +673,8 @@ void CommandObject::GenerateHelpText(Stream &output_strm) {
if (options != nullptr) {
options->GenerateOptionUsage(
output_strm, *this,
- GetCommandInterpreter().GetDebugger().GetTerminalWidth());
+ GetCommandInterpreter().GetDebugger().GetTerminalWidth(),
+ GetCommandInterpreter().GetDebugger().GetUseColor());
}
llvm::StringRef long_help = GetHelpLong();
if (!long_help.empty()) {
diff --git a/lldb/source/Interpreter/Options.cpp b/lldb/source/Interpreter/Options.cpp
index 4cf68db466158..ec725428483ff 100644
--- a/lldb/source/Interpreter/Options.cpp
+++ b/lldb/source/Interpreter/Options.cpp
@@ -19,6 +19,7 @@
#include "lldb/Interpreter/CommandObject.h"
#include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Target/Target.h"
+#include "lldb/Utility/AnsiTerminal.h"
#include "lldb/Utility/DiagnosticsRendering.h"
#include "lldb/Utility/StreamString.h"
#include "llvm/ADT/STLExtras.h"
@@ -261,7 +262,8 @@ Option *Options::GetLongOptions() {
void Options::OutputFormattedUsageText(Stream &strm,
const OptionDefinition &option_def,
- uint32_t output_max_columns) {
+ uint32_t output_max_columns,
+ bool use_color) {
std::string actual_text;
if (option_def.validator) {
const char *condition = option_def.validator->ShortConditionString();
@@ -278,7 +280,7 @@ void Options::OutputFormattedUsageText(Stream &strm,
if (static_cast<uint32_t>(actual_text.length() + strm.GetIndentLevel()) <
output_max_columns) {
// Output it as a single line.
- strm.Indent(actual_text);
+ strm.Indent(ansi::FormatAnsiTerminalCodes(actual_text, use_color));
strm.EOL();
} else {
// We need to break it up into multiple lines.
@@ -312,7 +314,8 @@ void Options::OutputFormattedUsageText(Stream &strm,
strm.Indent();
assert(start < final_end);
assert(start + sub_len <= final_end);
- strm.Write(actual_text.c_str() + start, sub_len);
+ strm.PutCString(ansi::FormatAnsiTerminalCodes(
+ llvm::StringRef(actual_text.c_str() + start, sub_len), use_color));
start = end + 1;
}
strm.EOL();
@@ -385,7 +388,7 @@ static bool PrintOption(const OptionDefinition &opt_def,
}
void Options::GenerateOptionUsage(Stream &strm, CommandObject &cmd,
- uint32_t screen_width) {
+ uint32_t screen_width, bool use_color) {
auto opt_defs = GetDefinitions();
const uint32_t save_indent_level = strm.GetIndentLevel();
llvm::StringRef name = cmd.GetCommandName();
@@ -527,7 +530,7 @@ void Options::GenerateOptionUsage(Stream &strm, CommandObject &cmd,
strm.IndentMore(5);
if (opt_def.usage_text)
- OutputFormattedUsageText(strm, opt_def, screen_width);
+ OutputFormattedUsageText(strm, opt_def, screen_width, use_color);
if (!opt_def.enum_values.empty()) {
strm.Indent();
strm.Printf("Values: ");
|
|
A bit surprised that this is gated by colour (or indeed, color) or not, but I see that the colour option is basically the ANSI option. Other settings imply this: So TIL that. I think style wise, even if it's the first character of a sentence it should match the case of the character you'd type in. Whereas in a GUI app we have the expectation that you just press the key: (the GUI apps in fact only look for lower case, alt+shift something does not work) Otherwise, I like it. Certainly leaves a trail for anyone wondering what it means. |
| Desc<"Name of the process ${ansi.underline}p${ansi.normal}lugin you " | ||
| "want to use.">; | ||
| def process_attach_pid : Option<"pid", "p">, | ||
| Group<1>, | ||
| Arg<"Pid">, | ||
| Desc<"The ${ansi.underline}p${ansi.normal}rocess ID " |
There was a problem hiding this comment.
both of these are underlining "p", but only one of these has the has the short option of -p.
this approach works great for short options of lower case letters, but can still be confusing for upper case short options.
| def process_attach_include_existing | ||
| : Option<"include-existing", "i">, | ||
| Group<2>, | ||
| Desc<"${ansi.underline}I${ansi.normal}nclude existing processes when " |
There was a problem hiding this comment.
related to my previous comment, this example underlines "I" but the short option is -i.
I think I may have conflated the two out of laziness when introducing column visualization. IMO, it does make sense to separate out "does the user want color?" from "does the terminal support escapes?". For example, a color-blind person might want underlines and bold text, but no colors. |
|
Regarding capitalization, I pretty strongly prefer not to change it and focus on highlighting the mnemonic. Here's why:
@DavidSpickett @kastiglione let me know if you folks feel differently or have an alternative suggestion. |
kastiglione
left a comment
kastiglione
left a comment
There was a problem hiding this comment.
The explanation makes sense, this is an improvement, thanks!
I'm fine with this option. The purpose was to help make the letter we're choosing for the short option stick in people's minds a bit when they look at the help output. underlining the letter does that all the way for lower case and gets close enough for upper case short options to be useful without making the output look weird. |
Then I'm cool with this on the grounds that it's following presumed existing practice. Tbf I don't think I've ever seen a CLI application do this highlighting, so there isn't a precedent. |
|
Created a PR to simplify the syntax in the tabelgen files: #155694 |
6 participants
Whenever an option would use something other than the first letter of the long option as the short option, Jim would capitalized the letter we picked as a mnemonic. This has often been mistaken for a typo and Jim wondered if we should stop doing this.
During the discussion, David mentioned how this reminds him of the underline in menu bars when holding down alt. I suggested we do something similar in LLDB by underlying the letter in the description.
Here's what that looks like for a subset of the
process attachoptions:See https://discourse.llvm.org/t/should-we-remove-the-capital-letter-in-option-helps/87816