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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:243
+    /// - Strict
+    std::optional<Located<std::string>> Mode;
+
----------------
kadircet wrote:
> sammccall wrote:
> > I think "Diagnostics.Mode" is too vague a name.
> > 
> > I expect this to be a rollout flag that we remove in the medium term (either deciding to stick to the old or new behavior), so maybe something ultra-specific like`AllowStalePreamble: yes/no`?
> > (Preamble is jargon, maybe there's something better, but again I don't think we should plan for this to be really "user-visible")
> i actually feel like there's some value in keeping this around. some users value correctness of their diagnostics too much, and option isn't really costly to implement. why do you think we should stick to one or the other?
> some users value correctness of their diagnostics too much

First, citation needed! (People *claiming* that they value correctness when they actually prefer latency seems common based on our past optimizations).

Second, this is a fine-grained knob that probably doesn't yield a useful correctness/latency tradeoff. If it trades off a lot of correctness, we probably want to leave it off by default and at that point we should delete the feature. If it trades off only a little correctness, then most people will want it and *disabling it won't significantly improve diagnostics correctness* as most errors come from non-configurable tradeoffs made elsewhere.

There's a sweet spot where this trades off just the critical amount of correctness to make it a difficult call, I just think it's unlikely we'll hit that. (I'm optimistic about this being mostly unnoticeable).

---

Regardless, if this is a long-lived option, it's **more** important to have a good name. `AllowStalePreamble` seems clearly better than `Mode` but maybe we could come up with something better - I just don't think it matters that much if the option is short-lived.


================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:555
+  EXPECT_TRUE(compileAndApply());
+  // Defaults to Strict.
+  EXPECT_EQ(Conf.Diagnostics.Mode, Config::DiagnosticsMode::Fast);
----------------
comment doesn't seem to apply here (and below)


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


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:628
+  AdditionalFiles["bar.h"] = "#pragma once";
+  llvm::StringLiteral BaselinePreamble("#define FOO 1\n[[#include \"foo.h\"]]");
+  {
----------------
nit: embedded newlines are hard to read and inconsistent with other tests. rawstrings?


================
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());
----------------
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)


================
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".
----------------
assert the behavior instead of describing it in a comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142890