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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The code looks good, but I have a very hard time following the tests.



================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:225
   auto TU = TestTU::withCode(Modified);
+  TU.AdditionalFiles = std::move(AdditionalFiles);
   auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
----------------
nit: seems clearer at this point to clone the TU and overwrite Code


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:624
+
+  llvm::StringLiteral BaselinePreamble = "#define FOO\n";
+  {
----------------
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


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:655
+  {
+    // Check with removals from preamble.
+    Annotations NewCode("[[#  include \"foo.h\"]]");
----------------
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?


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:656
+    // Check with removals from preamble.
+    Annotations NewCode("[[#  include \"foo.h\"]]");
+    auto AST = createPatchedAST(Annotations(BaselinePreamble).code(),
----------------
raw strings


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:664
+    Annotations Code(BaselinePreamble);
+    Annotations NewCode(("[[#include \"bar.h\"]]\n" + BaselinePreamble).str());
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
----------------
raw strings


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:673
+    // Check ranges for notes.
+    Annotations NewCode(("#define [[BARXYZ]] 1\n" + BaselinePreamble +
+                         "void foo();\n#define [[FOO]] 2")
----------------
raw strings


================
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.
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:680
+    EXPECT_THAT(mainFileDiagRanges(*AST),
+                UnorderedElementsAre(NewCode.ranges()[0], NewCode.ranges()[2]));
+  }
----------------
named ranges?


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:695
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: We should point at the NewCode.
+    EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
----------------
nit: at the correct coordinates in NewCode? (the diagnostic already points at NewCode)


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:584
+
+std::vector<clangd::Range> mainFileDiagRanges(const ParsedAST &AST) {
+  std::vector<clangd::Range> Result;
----------------
kadircet wrote:
> sammccall wrote:
> > hmm, this convert-then-assert-eq is harder to debug when there are unexpected diagnostics than matchers, I think. Might be better to follow what DiagnosticTests does here?
> not sure what you mean by that. when things go wrong this prints the expected and actual ranges, which is pretty much all we care about from this logic's perspective.
I care about the diagnostic messages/codes too, otherwise I can't understand what the assertions are about (especially on failure, but even just reading the code I can't tell what is being tested)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142890