njames93 marked 6 inline comments as done.
njames93 added inline comments.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:303
E.instantiate()->addCheckFactories(CTFactories);
- CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
- tidy::ClangTidyGlobalOptions(), Inputs.Opts.ClangTidyOpts));
+ CTContext.emplace(asClangTidyOptionsProvider(
+ Inputs.ClangTidyProvider ? Inputs.ClangTidyProvider :
emptyTidyProvider,
----------------
sammccall wrote:
> do you think we should just directly/eagerly create the clang-tidy config
> here, and wrap it in a provider that always returns it?
> (with a check that the requested filename matches, see other comment)
>
> Now that I understand why CTContext is lazy, I'm not sure we're benefitting
> from it.
Kind of how it is set already out with a DefaultOptionsProvider?
================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+ /*Priority=*/unsigned &,
+ /*CWD*/ PathRef);
+} // namespace detail
----------------
sammccall wrote:
> njames93 wrote:
> > 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.
> Ah, learn something new and horrifying every day!
>
> AFAICT the only thing that actually does this is is
> readability/IdentifierNamingCheck, which looks like it uses the style of the
> file where an identifier is declared to check its name.
> Our intent is very much that we're running clang-tidy strictly limited to the
> current file. The clangd config certainly doesn't support querying config for
> other files.
>
> I think we should really only expose the current file config, and return
> empty config when asked for any other file.
> We can have the TidyConfigAdapter do this, I think.
> (Slight catch: we want getConfigForFile("rel/path/to/current/file.cpp") to
> work, for IdentifierNamingCheck. I think if it's a relative path, we can just
> check whether the basename matches - shouldn't have too many matches)
That option in IdentifierNamingCheck is itself controlled by an option, we
could just subvert that option in disableUnusableChecks as a, somewhat dirty,
fix.
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
