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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 29 10:27:32 PDT 2019

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. 

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list