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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 4 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+                                   /*Priority=*/unsigned &,
+                                   /*CWD*/ PathRef);
+} // namespace detail
----------------
sammccall wrote:
> 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.
> and more likely to do the right thing if other such checks get added.
I'm going to change the identifier-naming option to make it a global. I can think of other checks in clang-tidy land that would benefit from it.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91029