sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice! Just readability/wording nits
================
Comment at: clangd/Diagnostics.cpp:463
+ // header.
+ auto ShouldAddDiag = [this](const Diag &D) {
+ if (mentionsMainFile(D))
----------------
ilya-biryukov wrote:
> Could you inline `ShouldAddDiag` into its single callsite?
Does this improve readability over (inline) `D.Severity >=
DiagnosticsEngine::Error`?
================
Comment at: clangd/Diagnostics.cpp:137
+ N.File = FE->getName();
+ N.Message = "Error origin";
+ N.Range = diagnosticRange(Info, LangOpts);
----------------
Clang tends to use a phrase ending in "here" for such notes.
Suggest "error occurred here"
================
Comment at: clangd/Diagnostics.cpp:137
+ N.File = FE->getName();
+ N.Message = "Error origin";
+ N.Range = diagnosticRange(Info, LangOpts);
----------------
sammccall wrote:
> Clang tends to use a phrase ending in "here" for such notes.
> Suggest "error occurred here"
message should not be capitalized (capitalization will be added later if needed)
================
Comment at: clangd/Diagnostics.cpp:141
+ // Update message to mention original file.
+ D.Message = (llvm::Twine("Error in include ",
+ llvm::sys::path::filename(FE->getName())) +
----------------
Include is not a noun. "included file"?
================
Comment at: clangd/Diagnostics.cpp:141
+ // Update message to mention original file.
+ D.Message = (llvm::Twine("Error in include ",
+ llvm::sys::path::filename(FE->getName())) +
----------------
sammccall wrote:
> Include is not a noun. "included file"?
"Error" is the severity, which appears elsewhere in LSP. (Other messages do not
include it)
================
Comment at: clangd/Diagnostics.cpp:141
+ // Update message to mention original file.
+ D.Message = (llvm::Twine("Error in include ",
+ llvm::sys::path::filename(FE->getName())) +
----------------
sammccall wrote:
> sammccall wrote:
> > Include is not a noun. "included file"?
> "Error" is the severity, which appears elsewhere in LSP. (Other messages do
> not include it)
message should not be capitalized
================
Comment at: clangd/Diagnostics.cpp:142
+ D.Message = (llvm::Twine("Error in include ",
+ llvm::sys::path::filename(FE->getName())) +
+ ": " + D.Message)
----------------
why are we including the filename here?
it obscures the error message, and will be available via the note.
================
Comment at: clangd/Diagnostics.h:141
llvm::Optional<Diag> LastDiag;
+ /// Stores lines of the includes containing a diagnostic.
+ llvm::DenseSet<int> IncludeHasDiag;
----------------
nit: "containing an error" (and variable name)
IncludeLinesWithErrors might be a clearer name
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59302/new/
https://reviews.llvm.org/D59302
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
