[lldb] Correct style of error messages #156774
Merged
+84
−84
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThe LLVM Style Guide says the following about error and warning messages [1]: > [T]o match error message styles commonly produced by other tools, I often provide this feedback during code review, but we still have a bunch of places where we have inconsistent error message, which bothers me as a user. This PR identifies a handful of those places and updates the messages to be consistent. [1] https://llvm.org/docs/CodingStandards.html#error-and-warning-messages Patch is 33.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/156774.diff 25 Files Affected:
diff --git a/lldb/source/API/SBCommandInterpreter.cpp b/lldb/source/API/SBCommandInterpreter.cpp
index 4ea79d336e08d..34323bc5a2c37 100644
--- a/lldb/source/API/SBCommandInterpreter.cpp
+++ b/lldb/source/API/SBCommandInterpreter.cpp
@@ -208,7 +208,7 @@ void SBCommandInterpreter::HandleCommandsFromFile(
LLDB_INSTRUMENT_VA(this, file, override_context, options, result);
if (!IsValid()) {
- result->AppendError("SBCommandInterpreter is not valid.");
+ result->AppendError("SBCommandInterpreter is not valid");
return;
}
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index 38ec375c03070..de0a7e7093411 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -609,12 +609,12 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
const size_t num_files = m_options.m_filenames.GetSize();
if (num_files == 0) {
if (!GetDefaultFile(target, file, result)) {
- result.AppendError("No file supplied and no default file available.");
+ result.AppendError("no file supplied and no default file available");
return;
}
} else if (num_files > 1) {
- result.AppendError("Only one file at a time is allowed for file and "
- "line breakpoints.");
+ result.AppendError("only one file at a time is allowed for file and "
+ "line breakpoints");
return;
} else
file = m_options.m_filenames.GetFileSpecAtIndex(0);
@@ -784,7 +784,7 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
}
result.SetStatus(eReturnStatusSuccessFinishResult);
} else if (!bp_sp) {
- result.AppendError("Breakpoint creation failed: No breakpoint created.");
+ result.AppendError("breakpoint creation failed: no breakpoint created");
}
}
@@ -940,7 +940,7 @@ class CommandObjectBreakpointEnable : public CommandObjectParsed {
size_t num_breakpoints = breakpoints.GetSize();
if (num_breakpoints == 0) {
- result.AppendError("No breakpoints exist to be enabled.");
+ result.AppendError("no breakpoints exist to be enabled");
return;
}
@@ -1048,7 +1048,7 @@ the second re-enables the first location.");
size_t num_breakpoints = breakpoints.GetSize();
if (num_breakpoints == 0) {
- result.AppendError("No breakpoints exist to be disabled.");
+ result.AppendError("no breakpoints exist to be disabled");
return;
}
@@ -1224,7 +1224,7 @@ class CommandObjectBreakpointList : public CommandObjectParsed {
}
result.SetStatus(eReturnStatusSuccessFinishNoResult);
} else {
- result.AppendError("Invalid breakpoint ID.");
+ result.AppendError("invalid breakpoint ID");
}
}
}
@@ -1318,7 +1318,7 @@ class CommandObjectBreakpointClear : public CommandObjectParsed {
// Early return if there's no breakpoint at all.
if (num_breakpoints == 0) {
- result.AppendError("Breakpoint clear: No breakpoint cleared.");
+ result.AppendError("breakpoint clear: no breakpoint cleared");
return;
}
@@ -1364,7 +1364,7 @@ class CommandObjectBreakpointClear : public CommandObjectParsed {
output_stream.EOL();
result.SetStatus(eReturnStatusSuccessFinishNoResult);
} else {
- result.AppendError("Breakpoint clear: No breakpoint cleared.");
+ result.AppendError("breakpoint clear: no breakpoint cleared");
}
}
@@ -1459,7 +1459,7 @@ class CommandObjectBreakpointDelete : public CommandObjectParsed {
size_t num_breakpoints = breakpoints.GetSize();
if (num_breakpoints == 0) {
- result.AppendError("No breakpoints exist to be deleted.");
+ result.AppendError("no breakpoints exist to be deleted");
return;
}
@@ -1504,7 +1504,7 @@ class CommandObjectBreakpointDelete : public CommandObjectParsed {
}
}
if (valid_bp_ids.GetSize() == 0) {
- result.AppendError("No disabled breakpoints.");
+ result.AppendError("no disabled breakpoints");
return;
}
} else {
@@ -1712,7 +1712,7 @@ class CommandObjectBreakpointNameConfigure : public CommandObjectParsed {
const size_t argc = command.GetArgumentCount();
if (argc == 0) {
- result.AppendError("No names provided.");
+ result.AppendError("no names provided");
return;
}
@@ -1799,7 +1799,7 @@ class CommandObjectBreakpointNameAdd : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
if (!m_name_options.m_name.OptionWasSet()) {
- result.AppendError("No name option provided.");
+ result.AppendError("no name option provided");
return;
}
@@ -1813,7 +1813,7 @@ class CommandObjectBreakpointNameAdd : public CommandObjectParsed {
size_t num_breakpoints = breakpoints.GetSize();
if (num_breakpoints == 0) {
- result.AppendError("No breakpoints, cannot add names.");
+ result.AppendError("no breakpoints, cannot add names");
return;
}
@@ -1825,7 +1825,7 @@ class CommandObjectBreakpointNameAdd : public CommandObjectParsed {
if (result.Succeeded()) {
if (valid_bp_ids.GetSize() == 0) {
- result.AppendError("No breakpoints specified, cannot add names.");
+ result.AppendError("no breakpoints specified, cannot add names");
return;
}
size_t num_valid_ids = valid_bp_ids.GetSize();
@@ -1873,7 +1873,7 @@ class CommandObjectBreakpointNameDelete : public CommandObjectParsed {
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
if (!m_name_options.m_name.OptionWasSet()) {
- result.AppendError("No name option provided.");
+ result.AppendError("no name option provided");
return;
}
@@ -1887,7 +1887,7 @@ class CommandObjectBreakpointNameDelete : public CommandObjectParsed {
size_t num_breakpoints = breakpoints.GetSize();
if (num_breakpoints == 0) {
- result.AppendError("No breakpoints, cannot delete names.");
+ result.AppendError("no breakpoints, cannot delete names");
return;
}
@@ -1899,7 +1899,7 @@ class CommandObjectBreakpointNameDelete : public CommandObjectParsed {
if (result.Succeeded()) {
if (valid_bp_ids.GetSize() == 0) {
- result.AppendError("No breakpoints specified, cannot delete names.");
+ result.AppendError("no breakpoints specified, cannot delete names");
return;
}
ConstString bp_name(m_name_options.m_name.GetCurrentValue());
diff --git a/lldb/source/Commands/CommandObjectCommands.cpp b/lldb/source/Commands/CommandObjectCommands.cpp
index 3049eb8c20dbc..a3293f0f7966d 100644
--- a/lldb/source/Commands/CommandObjectCommands.cpp
+++ b/lldb/source/Commands/CommandObjectCommands.cpp
@@ -418,7 +418,7 @@ other command as far as there is only one alias command match.");
if ((pos != std::string::npos) && (pos > 0))
raw_command_string = raw_command_string.substr(pos);
} else {
- result.AppendError("Error parsing command string. No alias created.");
+ result.AppendError("error parsing command string. No alias created");
return;
}
@@ -2888,7 +2888,7 @@ class CommandObjectCommandsContainerDelete : public CommandObjectParsed {
size_t num_args = command.GetArgumentCount();
if (num_args == 0) {
- result.AppendError("No command was specified.");
+ result.AppendError("no command was specified");
return;
}
diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index 7e58a95fae2c3..88a02dce35b9d 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -140,7 +140,7 @@ class CommandObjectFrameDiagnose : public CommandObjectParsed {
} else {
StopInfoSP stop_info_sp = thread->GetStopInfo();
if (!stop_info_sp) {
- result.AppendError("No arguments provided, and no stop info.");
+ result.AppendError("no arguments provided, and no stop info");
return;
}
@@ -148,7 +148,7 @@ class CommandObjectFrameDiagnose : public CommandObjectParsed {
}
if (!valobj_sp) {
- result.AppendError("No diagnosis available.");
+ result.AppendError("no diagnosis available");
return;
}
@@ -310,7 +310,7 @@ class CommandObjectFrameSelect : public CommandObjectParsed {
if (frame_idx == 0) {
// If you are already at the bottom of the stack, then just warn
// and don't reset the frame.
- result.AppendError("Already at the bottom of the stack.");
+ result.AppendError("already at the bottom of the stack");
return;
} else
frame_idx = 0;
@@ -335,7 +335,7 @@ class CommandObjectFrameSelect : public CommandObjectParsed {
if (frame_idx == num_frames - 1) {
// If we are already at the top of the stack, just warn and don't
// reset the frame.
- result.AppendError("Already at the top of the stack.");
+ result.AppendError("already at the top of the stack");
return;
} else
frame_idx = num_frames - 1;
diff --git a/lldb/source/Commands/CommandObjectLog.cpp b/lldb/source/Commands/CommandObjectLog.cpp
index 17efae189b05e..0c01da0b56834 100644
--- a/lldb/source/Commands/CommandObjectLog.cpp
+++ b/lldb/source/Commands/CommandObjectLog.cpp
@@ -547,7 +547,7 @@ class CommandObjectLogTimerIncrement : public CommandObjectParsed {
Timer::SetQuiet(!increment);
result.SetStatus(eReturnStatusSuccessFinishNoResult);
} else
- result.AppendError("Could not convert increment value to boolean.");
+ result.AppendError("could not convert increment value to boolean");
}
if (!result.Succeeded()) {
diff --git a/lldb/source/Commands/CommandObjectMultiword.cpp b/lldb/source/Commands/CommandObjectMultiword.cpp
index c99b75ff29144..a369557cca845 100644
--- a/lldb/source/Commands/CommandObjectMultiword.cpp
+++ b/lldb/source/Commands/CommandObjectMultiword.cpp
@@ -159,7 +159,7 @@ void CommandObjectMultiword::Execute(const char *args_string,
auto sub_command = args[0].ref();
if (sub_command.empty()) {
- result.AppendError("Need to specify a non-empty subcommand.");
+ result.AppendError("need to specify a non-empty subcommand");
return;
}
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index 84c576e721e71..7d326404a5503 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -259,7 +259,7 @@ class CommandObjectProcessLaunch : public CommandObjectProcessLaunchOrAttach {
if (!exe_module_sp)
exe_module_sp = target->GetExecutableModule();
if (!exe_module_sp) {
- result.AppendWarning("Could not get executable module after launch.");
+ result.AppendWarning("could not get executable module after launch");
} else {
const char *archname =
diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp
index 7e7d3f065b622..0b4599b16ef0d 100644
--- a/lldb/source/Commands/CommandObjectSource.cpp
+++ b/lldb/source/Commands/CommandObjectSource.cpp
@@ -513,7 +513,7 @@ class CommandObjectSourceInfo : public CommandObjectParsed {
"No selected frame to use to find the default source.");
return false;
} else if (!cur_frame->HasDebugInformation()) {
- result.AppendError("No debug info for the selected frame.");
+ result.AppendError("no debug info for the selected frame");
return false;
} else {
const SymbolContext &sc =
@@ -553,11 +553,11 @@ class CommandObjectSourceInfo : public CommandObjectParsed {
}
}
if (!m_module_list.GetSize()) {
- result.AppendError("No modules match the input.");
+ result.AppendError("no modules match the input");
return;
}
} else if (target.GetImages().GetSize() == 0) {
- result.AppendError("The target has no associated executable images.");
+ result.AppendError("the target has no associated executable images");
return;
}
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index 3ae08dec75e31..004542e3e6aed 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -2420,7 +2420,7 @@ class CommandObjectTargetModulesDumpLineTable
result.GetErrorStream().SetAddressByteSize(addr_byte_size);
if (command.GetArgumentCount() == 0) {
- result.AppendError("file option must be specified.");
+ result.AppendError("file option must be specified");
return;
} else {
// Dump specified images (by basename or fullpath)
@@ -3565,13 +3565,13 @@ class CommandObjectTargetModulesShowUnwind : public CommandObjectParsed {
ThreadList threads(process->GetThreadList());
if (threads.GetSize() == 0) {
- result.AppendError("The process must be paused to use this command.");
+ result.AppendError("the process must be paused to use this command");
return;
}
ThreadSP thread(threads.GetThreadAtIndex(0));
if (!thread) {
- result.AppendError("The process must be paused to use this command.");
+ result.AppendError("the process must be paused to use this command");
return;
}
diff --git a/lldb/source/Commands/CommandObjectThread.cpp b/lldb/source/Commands/CommandObjectThread.cpp
index 57c23d533fb96..bbec714642ec9 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -1570,7 +1570,7 @@ class CommandObjectThreadReturn : public CommandObjectRaw {
uint32_t frame_idx = frame_sp->GetFrameIndex();
if (frame_sp->IsInlined()) {
- result.AppendError("Don't know how to return from inlined frames.");
+ result.AppendError("don't know how to return from inlined frames");
return;
}
diff --git a/lldb/source/Commands/CommandObjectWatchpoint.cpp b/lldb/source/Commands/CommandObjectWatchpoint.cpp
index e79c3b8939fa6..12effed12a3cf 100644
--- a/lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -44,7 +44,7 @@ static bool CheckTargetForWatchpointOperations(Target &target,
bool process_is_valid =
target.GetProcessSP() && target.GetProcessSP()->IsAlive();
if (!process_is_valid) {
- result.AppendError("There's no process or it is not alive.");
+ result.AppendError("there's no process or it is not alive");
return false;
}
// Target passes our checks, return true.
@@ -243,7 +243,7 @@ class CommandObjectWatchpointList : public CommandObjectParsed {
std::vector<uint32_t> wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(
target, command, wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
@@ -298,7 +298,7 @@ class CommandObjectWatchpointEnable : public CommandObjectParsed {
size_t num_watchpoints = watchpoints.GetSize();
if (num_watchpoints == 0) {
- result.AppendError("No watchpoints exist to be enabled.");
+ result.AppendError("no watchpoints exist to be enabled");
return;
}
@@ -314,7 +314,7 @@ class CommandObjectWatchpointEnable : public CommandObjectParsed {
std::vector<uint32_t> wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(
target, command, wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
@@ -366,7 +366,7 @@ class CommandObjectWatchpointDisable : public CommandObjectParsed {
size_t num_watchpoints = watchpoints.GetSize();
if (num_watchpoints == 0) {
- result.AppendError("No watchpoints exist to be disabled.");
+ result.AppendError("no watchpoints exist to be disabled");
return;
}
@@ -385,7 +385,7 @@ class CommandObjectWatchpointDisable : public CommandObjectParsed {
std::vector<uint32_t> wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(
target, command, wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
@@ -476,7 +476,7 @@ class CommandObjectWatchpointDelete : public CommandObjectParsed {
size_t num_watchpoints = watchpoints.GetSize();
if (num_watchpoints == 0) {
- result.AppendError("No watchpoints exist to be deleted.");
+ result.AppendError("no watchpoints exist to be deleted");
return;
}
@@ -500,7 +500,7 @@ class CommandObjectWatchpointDelete : public CommandObjectParsed {
std::vector<uint32_t> wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(target, command,
wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
@@ -596,7 +596,7 @@ class CommandObjectWatchpointIgnore : public CommandObjectParsed {
size_t num_watchpoints = watchpoints.GetSize();
if (num_watchpoints == 0) {
- result.AppendError("No watchpoints exist to be ignored.");
+ result.AppendError("no watchpoints exist to be ignored");
return;
}
@@ -611,7 +611,7 @@ class CommandObjectWatchpointIgnore : public CommandObjectParsed {
std::vector<uint32_t> wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(
target, command, wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
@@ -715,7 +715,7 @@ class CommandObjectWatchpointModify : public CommandObjectParsed {
size_t num_watchpoints = watchpoints.GetSize();
if (num_watchpoints == 0) {
- result.AppendError("No watchpoints exist to be modified.");
+ result.AppendError("no watchpoints exist to be modified");
return;
}
@@ -728,7 +728,7 @@ class CommandObjectWatchpointModify : public CommandObjectParsed {
std::vector<uint32_t> wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(
target, command, wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
diff --git a/lldb/source/Commands/CommandObjectWatchpointCommand.cpp b/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
index 32cb80b421fd6..062bf75eb8ae8 100644
--- a/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
@@ -376,7 +376,7 @@ are no syntax errors may indicate that a function was declared but never called.
std::vector<uint32_t> valid_wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(target, command,
valid_wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
@@ -470,7 +470,7 @@ class CommandObjectWatchpointCommandDelete : public CommandObjectParsed {
std::vector<uint32_t> valid_wp_ids;
if (!CommandObjectMultiwordWatchpoint::VerifyWatchpointIDs(target, command,
valid_wp_ids)) {
- result.AppendError("Invalid watchpoints specification.");
+ result.AppendError("invalid watchpoints specification");
return;
}
@@ -525,7 +525,7 @@ class CommandObjectWatchpointCommandList : public CommandObjectParsed {
std::vector<uint32_t> valid_wp_ids;
if (!CommandO...
[truncated]
|
|
Great! May be we should do this for:
|
|
Also, it would be great if we had a clang-tidy checker as part of the CI testing to catching this inconsistencies earlier 👀 |
| @@ -418,7 +418,7 @@ other command as far as there is only one alias command match."); | |||
| if ((pos != std::string::npos) && (pos > 0)) | |||
| raw_command_string = raw_command_string.substr(pos); | |||
| } else { | |||
| result.AppendError("Error parsing command string. No alias created."); | |||
| result.AppendError("error parsing command string. No alias created"); | |||
There was a problem hiding this comment.
If it is multiple sentences, does the guidance say anything about that?
There was a problem hiding this comment.
It does, yeah, it even includes an example (which I know you couldn't see because of the infra issue):
error: file.o: section header 3 is corrupt. Size is 10 when it should be 20
The double spaces is something else we should settle on, but that's a separate discussion...
The LLVM Style Guide says the following about error and warning messages [1]: > [T]o match error message styles commonly produced by other tools, > start the first sentence with a lowercase letter, and finish the last > sentence without a period, if it would end in one otherwise. I often provide this feedback during code review, but we still have a bunch of places where we have inconsistent error message, which bothers me as a user. This PR identifies a handful of those places and updates the messages to be consistent. [1] https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
e06f382 to
7e2901b
Compare
Yeah, those are all good functions to grep for.
It would be nice to automate. Currently, we don't run any clang-tidy checks in CI and I think it would be most valuable if that were part of the pre-commit checks. The clang-tidy checks are a lot more expensive than the formatters though, so I don't know if that's even on the table. Might be an interesting topic for the infa workgroup. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The LLVM Style Guide says the following about error and warning messages [1]:
I often provide this feedback during code review, but we still have a bunch of places where we have inconsistent error message, which bothers me as a user. This PR identifies a handful of those places and updates the messages to be consistent.
[1] https://llvm.org/docs/CodingStandards.html#error-and-warning-messages