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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 5 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:555
+  EXPECT_TRUE(compileAndApply());
+  // Defaults to Strict.
+  EXPECT_EQ(Conf.Diagnostics.Mode, Config::DiagnosticsMode::Fast);
----------------
sammccall wrote:
> comment doesn't seem to apply here (and below)
oops, copy pasting is hard. just dropped them.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:584
+
+std::vector<clangd::Range> mainFileDiagRanges(const ParsedAST &AST) {
+  std::vector<clangd::Range> Result;
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:634
+                                NewCode.code(), AdditionalFiles);
+    // FIXME: Should be pointing at the range inside the Newcode.
+    EXPECT_THAT(mainFileDiagRanges(*AST), IsEmpty());
----------------
sammccall wrote:
> it's unclear what the behavior is here: do we get a diagnostic with no range, or a bad range that's being filtered out, or no diagnostic at all?
> 
> (The presence/absence of the diagnostics is important to test, independent of the locations)
well, sent out D143197. we were not generating any diagnositcs at all, because we couldn't figure out the headerid for patched include. i guess these improvements/fixes to the preamble patching deserves its own patch, whether or not we chose to improve things.


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:642
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    // FIXME: This is just all sorts of wrong. We point at the FOO from baseline
+    // claiming it's redefined and not point at the new include of "bar.h".
----------------
sammccall wrote:
> assert the behavior instead of describing it in a comment?
done. this test was checking so many things at once. made it simpler by getting rid of the macro def from the preamble and checking for it (the #undef issue) only in the next test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142890