sammccall planned changes to this revision.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:518
+
+ if (F.SlowChecks.has_value())
+ Out.Apply.push_back([V = **F.SlowChecks](const Params &, Config &C) {
----------------
kadircet wrote:
> nit: drop `has_value`?
This is a boolean, so there's an obvious+wrong interpretation of that version.
I think `has_value`, uh, has value here.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:286
bool isRegisteredTidyCheck(llvm::StringRef Check) {
assert(!Check.empty());
----------------
kadircet wrote:
> should we update callers here to also emit a warning when they're turning on
> a slow check (and possibly mention SlowChecks override?) this might as well
> be our way of testing this to some extent. we'd still rely on a certain
> check-name always being part of the list (and pick a new element whenever
> we're updating the list), but at least we wouldn't rely on semantics of the
> check (i.e. also have a test case that'd trigger the warning).
Callers here == diagnostics for config files that name unknown checks.
I can add a warning for naming a slow check too, that makes sense.
> this might as well be our way of testing this to some extent
Yeah, `ASSERT_FALSE(isFastCheck("misc-const-completeness"))` and a
corresponding positive case has some value even if we have to trivially update
it sometimes.
I'm less sure about setting up a complicated end-to-end case that we'll have to
rewrite/delete if the list changes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138505/new/
https://reviews.llvm.org/D138505
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
