ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. Many thanks for fixing this.
================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:111
-void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
+bool adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
const LangOptions &LangOpts) {
----------------
NIT: add a small comment mentioning that returned value indicates if `Diag` was
changed
================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:475
bool InsideMainFile = isInsideMainFile(Info);
+ SourceManager &SM = Info.getSourceManager();
----------------
NIT: could use `auto &` here, the `SourceManager` is mentioned explicitly in
the initializer anyway.
================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:563
+ LastDiagWasAdjusted =
+ !InsideMainFile && adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
----------------
NIT: I suggest moving the control-flow (`&&`) into a separate `if` statement.
`adjustDiagFromHeader` mutates its arguments and having complex expressions in
the same statement makes it a little hard to notice that things might get
mutated there.
But up to you.
================
Comment at: clang-tools-extra/clangd/Diagnostics.h:148
llvm::Optional<Diag> LastDiag;
+ // Set by adjustDiagFromHeader to indicate the fact that LastDiag was not
+ // inside main file initially.
----------------
NIT: it's not `adjustDiagFromHeader` that actually sets the value, maybe
rephrase to avoid confusion?
Something like
```
/// Set iff adjustDiagFromHeader resulted in changes to LastDiag.
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64863/new/
https://reviews.llvm.org/D64863
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
