[PATCH] D142890: [clangd] Add config option for fast diagnostics mode

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:624
+
+  llvm::StringLiteral BaselinePreamble = "#define FOO\n";
+  {
----------------
sammccall wrote:
> nit: "preamble" vs "code" is a confusing distinction when we're using both as code.
> Spelling out the inputs would be IMO clearer than pasting strings together, tests don't need to be DRY
>Spelling out the inputs would be IMO clearer than pasting strings together, tests don't need to be DRY

Done.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:655
+  {
+    // Check with removals from preamble.
+    Annotations NewCode("[[#  include \"foo.h\"]]");
----------------
sammccall wrote:
> what exactly is being tested here?
> "removals" suggests we're dropping includes but think we're not.
> Is the comment change significant, or the whitespace change in the directive, or both?
> The code looks OK, what's the diagnostic being emitted?
> what exactly is being tested here?

so this test is all about diagnostics that are emitted as part of the main file AST built, specifically the ones that are emitted on ranges inside the preamble.
the particular diagnostics on include directives are coming from include-cleaner.

> "removals" suggests we're dropping includes but think we're not.

idea is checking that translation logic works as intended when the preamble bounds got smaller.

> Is the comment change significant, or the whitespace change in the directive, or both?

comment change is insignificant, it's just to make sure we are making preamble bounds smaller, could as well be another include.
whitespace change is insignificant since we're moving the whole directive around (2nd line to 0th line). it was to make it more explicit. happy to hide if you think that's confusing.

> The code looks OK, what's the diagnostic being emitted?

unused-include diag as mentioned above. couldn't really find any diagnostics that are emitted (or contain notes that are pointing) to an include directive.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:677
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    // FIXME: We shouldn't be warning for BARXYZ, and pointing at the FOO inside
+    // the baselinepreamble.
----------------
sammccall wrote:
> what is the warning for BARXYZ?
> 
> This comment says the diagnostic shouldn't be pointing at FOO inside baselinepreamble (which I assume means the first FOO) but it already isn't - did you mean "should"? This is probably just two FIXMEs.
> what is the warning for BARXYZ?

`redefinition of FOO`, it's emitted at location of `B` then we turn it into a range inside clangd by lexing the token. hence it's expanded into the full token range.

> This comment says the diagnostic shouldn't be pointing at FOO inside baselinepreamble (which I assume means the first FOO) but it already isn't - did you mean "should"? This is probably just two FIXMEs.

sorry that's my bad english. i was trying to say it shouldn't point at `BARXYZ` and **should** point at `FOO` instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142890/new/

https://reviews.llvm.org/D142890