[PATCH] D91029: [clangd] Implement clang-tidy options from config

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK, I would love to get this landed.
I think the simplest thing is to compute the config eagerly and load it in with DefaultOptionsProvider as you mentioned.

I might fiddle with checking the filename afterwards, but I don't think this is blocking.
Similarly I want to add the IO caching after this lands.



================
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,
----------------
njames93 wrote:
> 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?
Right! The only wrinkle would be the use of empty options vs same options when requesting config for a different file.
But happy to go with DefaultOptionsProvider in to get this landed!


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+                                   /*Priority=*/unsigned &,
+                                   /*CWD*/ PathRef);
+} // namespace detail
----------------
njames93 wrote:
> 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.
Yeah, that has slightly different behavior though: it means that it will assume every other file has the same config as the current file. (i.e. the check *is* enabled for those files, and the style is the same).
Returning an empty config seems more principled, and more likely to do the right thing if other such checks get added.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91029/new/

https://reviews.llvm.org/D91029