sammccall added a comment.
OK this looks really great, thanks so much for persisting with this.
Comments are mostly simple nits/simplifications, with the exception of `Order`
which is a... slightly trickier simplification, but seems worth doing.
Tomorrow I'll adapt our internal clang-tidy config bits to work with this
interface...
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:241
+/// Empty clang tidy provider, using this as a provider will disable
clang-tidy.
+static void emptyTidyProvider(tidy::ClangTidyOptions &, llvm::StringRef,
+ unsigned &, PathRef) {}
----------------
or just use fixedTidyProvider("-*")
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:74
+ auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults();
+ EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
+ EmptyDefaults.User = llvm::sys::Process::GetEnv("USER");
----------------
I'm not sure if this applies anymore - it's going to be the bottom of the
stack, and nullopt is the default
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:75
+ EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
+ EmptyDefaults.User = llvm::sys::Process::GetEnv("USER");
+#ifdef _WIN32
----------------
nit: hoist the getenv out of the lambda?
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:75
+ EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
+ EmptyDefaults.User = llvm::sys::Process::GetEnv("USER");
+#ifdef _WIN32
----------------
sammccall wrote:
> nit: hoist the getenv out of the lambda?
seems like we can just assign to Opts.User directly?
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:99
+ "misc-definitions-in-headers");
+ Opts.Checks.emplace(DefaultChecks);
+ }
----------------
nit: = seems clearer than emplace
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:110
+ tidy::ClangTidyOptions &Opts, llvm::StringRef, unsigned &, PathRef) {
+ mergeCheckList(Opts.Checks, Checks);
+ mergeCheckList(Opts.WarningsAsErrors, WarningsAsErrors);
----------------
I think should just be assignment rather than merge (same with
WarningsAsErrors).
The name suggests that it always returns the same set of checks.
(Failing that, maybe call it `addTidyChecks`)
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:167
+
+ if (FS->makeAbsolute(AbsolutePath))
+ return;
----------------
(as noted elsewhere, the path is always absolute in clangd and we should make
this an interface requiremnet)
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:170
+
+ llvm::sys::path::remove_dots(AbsolutePath, true);
+ llvm::StringRef Directory = llvm::sys::path::parent_path(AbsolutePath);
----------------
I don't think we need to try to stat the directory here.
FileOptionsProvider does, but looking at the history, this seems related to
some combination of:
- arg validation (the parameter there is a directory name, rather than a
filename)
- trying to return an appropriate error_code (original return type was
ErrorOr<Options>)
- iterating over multiple possible config files
- maybe caching
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:181
+ }
+
+ for (llvm::StringRef CurrentDirectory = Directory;
----------------
can you add a FIXME to add caching here?
I hadn't realized FileOptionsProvider *was* actually doing caching.
(I don't think this is a big problem in the short term, and I don't think we
should solve it inline in this patch, rather I should land D88172 and use that,
which doesn't have the annoying "cache forever" behavior)
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:194
+ for (auto &Option : llvm::reverse(OptionStack))
+ Opts.mergeWith(Option, ++Order);
+ };
----------------
This order business is a real tangle, and looks like legacy to me (qualified vs
unqualified names).
I don't really think we want to use/expose it beyond the requirement that we
don't change the meaning of existing sets of `.clang-tidy` config files.
And I don't think we want to bother teaching clangd's config system about it.
So I'd suggest a hack/simplification:
- we remove the `order` parameter from TidyProvider
- here in provideClangTidyFiles, we just use a local Order which counts up
from 1. This preserves the relative precedence of .clang-tidy files
- in provideClangdConfig, we always set the order to max_uint (always wins)
- if we ever add checkoptions to any other providers, we set the order to 0
(always loses)
- combine() doesn't touch order
This *does* duplicate the fact that .clangd config is stacked on top of
.clang-tidy in ClangdMain, but in a way that only matters with disputes between
qualified/unqualified names.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:209
+
+class TidyProviderInstance : public tidy::ClangTidyOptionsProvider {
+public:
----------------
nit: "adapter" may be clearer than "instance", up to you
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+ /*Priority=*/unsigned &,
+ /*CWD*/ PathRef);
+} // namespace detail
----------------
we should always be passing an absolute path, so CWD shouldn't be needed.
I'm a bit surprised that ParsedAST::build doesn't assert that (though it calls
PreamblePatch::create, which does). Feel free to add it!
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:166
+list<std::string> DisabledClangTidyChecks{
+ "disable-clang-tidy-checks", cat(Features),
----------------
ah, sorry, I led you astray here... I don't think we actually need a flag to
control this in ClangdMain.cpp, just that the layering allows one :-)
Google's production deployment of clangd has a different entrypoint (not
ClangdMain.cpp) which has a flag to disable checks. A previous version of this
patch buried the list of disabled checks very deeply so that it couldn't be
changed, but building the TidyProvider stack in clangdmain addresses this (our
bizarro_clangd_main.cpp can do its own version making use of its flag).
You can keep this if you like, but I don't think it's necessary (for debugging
we can just use -clang-tidy-checks, and for users working around bugs we can
use clangd config).
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:850
+ Providers.push_back(provideEnvironment());
+ Providers.push_back(provideClangTidyFiles(TFS));
+ if (EnableConfig)
----------------
if -clang-tidy-checks is provided, we don't want to parse .clang-tidy files,
respect clangd config, or disable unusable checks - just environment + the flag.
The main purpose of that flag is to debug e.g. isolate crashes down to
individual checks, so full control is best.
(I believe this matches the old behavior, but it's really hard to tell from the
code - the new code is much nicer!)
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:853
+ Providers.push_back(provideClangdConfig());
+ if (ClangTidyChecks.empty())
+ Providers.push_back(fixedTidyProvider(ClangTidyChecks));
----------------
I think the condition is backwards
================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1221
+ // run.
+ FS.Files[testPath(".clang-tidy")] = R"(
+ Checks: -*,bugprone-use-after-move,llvm-header-guard
----------------
This is confusing... I think these are now disabled twice (by
disableUnusableChecks(), and via provideClangTidyFiles).
(I've got no objection to checking that elements of the standard tidy config
stack work together properly, but I don't think that's what's happening in this
test)
================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1226
+ std::vector<TidyProvider> Stack;
+ Stack.push_back(provideEnvironment());
+ Stack.push_back(provideClangTidyFiles(FS));
----------------
I think we probably don't want the environment here, if it does anything at all
it probably makes the test non-hermetic
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:133
auto TU = TestTU::withCode(Test.code());
- TU.ClangTidyChecks = "-*,google-explicit-constructor";
+ TU.ClangTidyProvider = fixedTidyProvider("-*,google-explicit-constructor");
EXPECT_THAT(
----------------
We sholudn't need -* here and elsewhere. This is the whole config provider,
nothing could be turning any checks on.
================
Comment at: clang-tools-extra/clangd/unittests/TestTU.cpp:62
Inputs.Opts = ParseOptions();
- Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
- Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors;
+ if (ClangTidyProvider)
+ Inputs.ClangTidyProvider = ClangTidyProvider;
----------------
I think the condition is not needed
================
Comment at: clang-tools-extra/clangd/unittests/TestTU.h:62
- llvm::Optional<std::string> ClangTidyChecks;
- llvm::Optional<std::string> ClangTidyWarningsAsErrors;
+ mutable TidyProvider ClangTidyProvider = {};
// Index to use when building AST.
----------------
mutable here smells funny, can we do `using TidyProvider =
unique_function<void(args) const>` instead? (so it's const-callable)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91029/new/
https://reviews.llvm.org/D91029
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
