[lldb] Remove redundant severity substring within a diagnostic message. #76111

PortalPete · 2023-12-21T01:00:42Z

For example, the following message has the severity string "error: " twice.

"error: :3:1: error: cannot find 'bogus' in scope

This method already appends the severity string in the beginning, but with this fix, it also removes a secondary instance, if applicable.

Note that this change only removes the first redundant substring. I considered putting the removal logic in a loop, but I decided that if something is generating more than one redundant severity substring, then that's a problem the message's source should probably fix.

rdar://114203423

llvmbot · 2023-12-21T01:01:49Z

@llvm/pr-subscribers-lldb

Author: Pete Lawrence (PortalPete)

Changes

For example, the following message has the severity string "error: " twice.
> "error: <EXPR>:3:1: error: cannot find 'bogus' in scope

This method already appends the severity string in the beginning, but with this fix, it also removes a secondary instance, if applicable.

Note that this change only removes the first redundant substring. I considered putting the removal logic in a loop, but I decided that if something is generating more than one redundant severity substring, then that's a problem the message's source should probably fix.

rdar://114203423

Full diff: https://github.com/llvm/llvm-project/pull/76111.diff

2 Files Affected:

(modified) lldb/source/Expression/DiagnosticManager.cpp (+11-2)
(modified) lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py (+1-1)

diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp
index 08977066e3330a..d9452328e5a27f 100644
--- a/lldb/source/Expression/DiagnosticManager.cpp
+++ b/lldb/source/Expression/DiagnosticManager.cpp
@@ -48,8 +48,17 @@ std::string DiagnosticManager::GetString(char separator) {
   std::string ret;
 
   for (const auto &diagnostic : Diagnostics()) {
-    ret.append(StringForSeverity(diagnostic->GetSeverity()));
-    ret.append(std::string(diagnostic->GetMessage()));
+    std::string message(diagnostic->GetMessage());
+    std::string searchable_message(diagnostic->GetMessage().lower());
+    std::string severity(StringForSeverity(diagnostic->GetSeverity()));
+
+    // Erase the (first) reduntant severity string in the message.
+    size_t position = searchable_message.find(severity);
+    if (position != std::string::npos)
+      message.erase(position, severity.length());
+
+    ret.append(severity);
+    ret.append(message);
     ret.push_back(separator);
   }
 
diff --git a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
index 36e302be2525b5..620b6e44fc852a 100644
--- a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
+++ b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
@@ -21,7 +21,7 @@ def test(self):
             "expr @import LLDBTestModule",
             error=True,
             substrs=[
-                "module.h:4:1: error: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
+                "module.h:4:1: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
                 "syntax_error_for_lldb_to_find // comment that tests source printing",
                 "could not build module 'LLDBTestModule'",
             ],

felipepiovezan

I'll leave the approval to others, but I left a coding suggestion.

It would be a lot nicer if we could tell the producer of those messages to not include the severity, but I think this is waaaaay out of scope here. The "severity" inside the message is likely coming from a producer that is too far gone at this point.

felipepiovezan · 2023-12-21T12:26:55Z

lldb/source/Expression/DiagnosticManager.cpp

+      message.erase(position, severity.length());
+
+    ret.append(severity);
+    ret.append(message);


I'm not sure how hot this code path is, but we are doing a bunch of unnecessary string copies in here (as we were before this PR as well...). For example, message and severity are both StringRefs that don't need to be converted into strings.

Suggestion:

std::string DiagnosticManager::GetString(char separator) { std::string ret; llvm::raw_string_ostream stream(ret); for (const auto &diagnostic : Diagnostics()) { llvm::StringRef severity = StringForSeverity(diagnostic->GetSeverity()); stream << severity; llvm::StringRef message = diagnostic->GetMessage(); std::string searchable_message = message.lower(); auto severity_pos = message.find_first_of(severity); stream << message.take_front(severity_pos); if (severity_pos != llvm::StringRef::npos) stream << message.drop_front(severity_pos + severity.size()); stream << separator; } return ret; }

I thought StringRef was better for strings that get passed around and reused and std::string is better for transient work, such as this.

Is general, is there an advantage to using StringRef over std::string?

I don't know how hot it is either but, I do know UserExpression.cpp has 2 call sites, which may be a popular path.

There's about 15 call sites from these files:

lldb/include/lldb/Expression/DiagnosticManager.h

lldb/source/Breakpoint/BreakpointLocation.cpp

lldb/source/Expression/DiagnosticManager.cpp

lldb/source/Expression/UserExpression.cpp

lldb/source/Expression/UtilityFunction.cpp

lldb/source/Interpreter/CommandReturnObject.cpp

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp

lldb/source/Target/Target.cpp

lldb/source/Utility/Status.cpp

The one from DiagnosticManager.h is its Dump() method which itself has 7 call sites.

I have a future PR that'll move this functionality into class Status, and whatever we decided to do here we can do there as well.

I thought StringRef was better for strings that get passed around and reused and std::string is better for transient work, such as this.

Is general, is there an advantage to using StringRef over std::string?

A StringRef is a non-owning view into a string. When we see one of those, that means the author of the code is making a few promises:

They don't need to mutate the string

They don't need to extend its lifetime past "the current" context.

Conversely, when the author conjures a std::string out of StringRef, the author is saying that they intend to break one of those promises. Note that your code doesn't need to, with the exception searchable_message: we had to modify the string with "to_lower"!

Oh, also ret breaks both promises: we modify it a lot and we also extend its lifetime past the current function, thus it cannot be a StringRef

Ok, you're suggestion is up @felipepiovezan.
Thanks again! 🙂

For example, the following message has the severity string "error: " twice. > "error: <EXPR>:3:1: error: cannot find 'bogus' in scope This method already appends the severity string in the beginning, but with this fix, it also removes a secondary instance, if applicable. Note that this change only removes the *first* redundant substring. I considered putting the removal logic in a loop, but I decided that if something is generating more than one redundant severity substring, then that's a problem the message's source should probably fix. rdar://114203423

felipepiovezan

LGTM! Thanks for trying out my suggestions!

PortalPete · 2024-01-18T19:16:37Z

Ping to @adrian-prantl, I believe this is good to go. 🙂

…e. (llvm#76111) For example, the following message has the severity string "error: " twice. > "error: <EXPR>:3:1: error: cannot find 'bogus' in scope This method already appends the severity string in the beginning, but with this fix, it also removes a secondary instance, if applicable. Note that this change only removes the *first* redundant substring. I considered putting the removal logic in a loop, but I decided that if something is generating more than one redundant severity substring, then that's a problem the message's source should probably fix. rdar://114203423

PortalPete requested a review from JDevlieghere as a code owner December 21, 2023 01:01

llvmbot added the lldb label Dec 21, 2023

PortalPete force-pushed the error-reporting/remove-double-error branch from dde9d02 to b87fd5c Compare December 21, 2023 01:05

felipepiovezan reviewed Dec 21, 2023

View reviewed changes

PortalPete force-pushed the error-reporting/remove-double-error branch from b87fd5c to d10ce06 Compare January 8, 2024 21:02

felipepiovezan approved these changes Jan 9, 2024

View reviewed changes

adrian-prantl merged commit c82b7fd into llvm:main Jan 18, 2024

PortalPete mentioned this pull request Feb 7, 2024

[lldb] Remove redundant severity substring within a diagnostic message swiftlang/llvm-project#8140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] Remove redundant severity substring within a diagnostic message. #76111

[lldb] Remove redundant severity substring within a diagnostic message. #76111

Uh oh!

PortalPete commented Dec 21, 2023 •

edited

Loading

Uh oh!

llvmbot commented Dec 21, 2023

Uh oh!

felipepiovezan left a comment

Uh oh!

felipepiovezan Dec 21, 2023

Uh oh!

PortalPete Dec 21, 2023

Uh oh!

PortalPete Dec 21, 2023

Uh oh!

felipepiovezan Dec 21, 2023

Uh oh!

felipepiovezan Dec 21, 2023 •

edited

Loading

Uh oh!

PortalPete Jan 8, 2024

Uh oh!

felipepiovezan left a comment

Uh oh!

PortalPete commented Jan 18, 2024

Uh oh!

Uh oh!

[lldb] Remove redundant severity substring within a diagnostic message. #76111

[lldb] Remove redundant severity substring within a diagnostic message. #76111

Uh oh!

Conversation

PortalPete commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 21, 2023

Uh oh!

felipepiovezan left a comment

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

PortalPete Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

PortalPete Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PortalPete Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

felipepiovezan left a comment

Choose a reason for hiding this comment

Uh oh!

PortalPete commented Jan 18, 2024

Uh oh!

Uh oh!

PortalPete commented Dec 21, 2023 •

edited

Loading

felipepiovezan Dec 21, 2023 •

edited

Loading