scott.linder added a comment.
Thank you for the patch! I will leave it to others with more knowledge of the
history of diagnostics to comment on whether the premise is acceptable, but I
have a few technical comments
================
Comment at: clang/lib/Frontend/TextDiagnosticPrinter.cpp:119-135
+ // If the diagnostic message is formatted into multiple lines, split the
+ // first line and feed it into printDiagnosticOptions so that we print the
+ // options only on the first line instead of the last line.
+ SmallString<100> InitStr;
+ size_t NewlinePos = OutStr.find_first_of('\n');
+ if (NewlinePos != StringRef::npos) {
+ for (size_t I = 0; I != NewlinePos; ++I)
----------------
Nit: I'd remove the `if`s and just use the behavior of `slice(npos, ...)`
returning the empty string
I also would reword "split the first line and feed it into
printDiagnosticOptions", as it could be interpreted as meaning the first line
is an input to `printDiagnosticOptions`
================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:99-103
+ size_t NewlinePos = PD->getShortDescription().find_first_of('\n');
+ if (NewlinePos != std::string::npos)
+ OutStr = PD->getShortDescription().str().insert(NewlinePos,
WarningMsg);
+ else
+ OutStr = (PD->getShortDescription() + WarningMsg).str();
----------------
It's frustrating that `std::string::insert` doesn't follow the pattern of
`npos==end-of-string`, but I'd still suggest only doing the actual
insert/append once
================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:100
+ size_t NewlinePos = PD->getShortDescription().find_first_of('\n');
+ if (NewlinePos != std::string::npos)
+ OutStr = PD->getShortDescription().str().insert(NewlinePos,
WarningMsg);
----------------
I believe they are guaranteed to be equivalent, but this should be
`StringRef::npos` or you should allocate the `std::string` sooner
================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:101
+ if (NewlinePos != std::string::npos)
+ OutStr = PD->getShortDescription().str().insert(NewlinePos,
WarningMsg);
+ else
----------------
I think you incur an extra copy assignment here, as the `insert` returns an
lvalue-reference
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127923/new/
https://reviews.llvm.org/D127923
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
