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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
