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

[Nathan James via Phabricator via cfe-commits]

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