njames93 marked 12 inline comments as done and an inline comment as not done.
njames93 added inline comments.
================
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) {}
----------------
sammccall wrote:
> or just use fixedTidyProvider("-*")
Issue with that approach is fixedTidyProvider has state so that would need a
stable storage when passing to `asClangTidyOptionsProvider`.
================
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");
----------------
sammccall wrote:
> I'm not sure if this applies anymore - it's going to be the bottom of the
> stack, and nullopt is the default
This is just incase `ClangTidyOptions::getDefaults()` ever changes.
================
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:
> sammccall wrote:
> > nit: hoist the getenv out of the lambda?
> seems like we can just assign to Opts.User directly?
We can't as `EmptyDefaults` also contains the Options clang tidy modules may
want to set.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:167
+
+ if (FS->makeAbsolute(AbsolutePath))
+ return;
----------------
sammccall wrote:
> (as noted elsewhere, the path is always absolute in clangd and we should make
> this an interface requiremnet)
See previous comment about clang-tidy checks not adhering to this requirement.
================
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);
----------------
sammccall wrote:
> 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
Ditto
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:194
+ for (auto &Option : llvm::reverse(OptionStack))
+ Opts.mergeWith(Option, ++Order);
+ };
----------------
sammccall wrote:
> 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.
That approach is a simplification but it only makes sense in how we are using
the providers in clangd. Downstream someone may want to add another provider
that may get ran after `.clangd` providers. Forcing `.clangd` to have the
highest priority will result in unexpected behaviour in that instance.
I'm happy to go with your approach if you don't see it as a blocking issue.
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+ /*Priority=*/unsigned &,
+ /*CWD*/ PathRef);
+} // namespace detail
----------------
sammccall wrote:
> 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!
While clangd may always use absolute paths, clang-tidy may not. Checks are able
to query the context and get options for other files.
When I put asserts on the Filename being absolute, things started failing.
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:850
+ Providers.push_back(provideEnvironment());
+ Providers.push_back(provideClangTidyFiles(TFS));
+ if (EnableConfig)
----------------
sammccall wrote:
> 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!)
The old behaviour, and the behaviour of the `-checks` option in clang-tidy is
to append the checks specified on the command line to whats found in
.clang-tidy files. This is so that the checks will respect other fields in the
configuration.
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:853
+ Providers.push_back(provideClangdConfig());
+ if (ClangTidyChecks.empty())
+ Providers.push_back(fixedTidyProvider(ClangTidyChecks));
----------------
sammccall wrote:
> I think the condition is backwards
Yes it was, good spot.
================
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
----------------
sammccall wrote:
> 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)
`provideClangTidyFiles` here will enable use-after-move/header-guard but
disable all other checks, which is what we want.
Hopefully `disableUnusableChecks` will then disable those 2 checks and all will
work well.
================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1226
+ std::vector<TidyProvider> Stack;
+ Stack.push_back(provideEnvironment());
+ Stack.push_back(provideClangTidyFiles(FS));
----------------
sammccall wrote:
> I think we probably don't want the environment here, if it does anything at
> all it probably makes the test non-hermetic
I agree, can trim this down a little.
================
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;
----------------
sammccall wrote:
> I think the condition is not needed
I think it is, the operator= for function_ref doesn't seem to correctly handle
the case when the argument doesn't hold a callable. Well asserts were failing
before I added the condition, may be a bug somewhere else.
================
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.
----------------
sammccall wrote:
> mutable here smells funny, can we do `using TidyProvider =
> unique_function<void(args) const>` instead? (so it's const-callable)
I was debating between the two but that does look a little nicer.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91029/new/
https://reviews.llvm.org/D91029
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
