[PATCH] D64863: [clangd] Ignore diags from builtin files

[Ilya Biryukov via Phabricator via cfe-commits]

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