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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
