Page MenuHomePhabricator

[clang-tidy] Add support for loading configs for specific files.
Needs ReviewPublic

Authored by njames93 on Nov 26 2020, 6:09 AM.

Details

Summary

There was already a basic version of this in IdentifierNamingCheck, controlled by an option GetConfigPerFile. This has now been changed to a global option UsePerFileConfig. As there hasn't been a release with GetConfigPerFile, This change shouldn't break anyones config.

Now there is support for this built into the base class directly, along with some basic caching.
As the clang-tidy providers used only have at best directory resolution, this caches based on directory rather than filename. This can potentially save alot of redundant config lookups.

Diff Detail

Event Timeline

njames93 created this revision.Nov 26 2020, 6:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2020, 6:09 AM
njames93 requested review of this revision.Nov 26 2020, 6:09 AM

I get the motivation for wanting to have a good, accessible way to do this thing that is possible today, but at the same time... I'm not sure we should pave this cow path, or even keep it.

Clang-tidy's configuration system is *really* complicated, and I don't think the complexity justified by the value it brings.
Currently AFAICS there's just *one* check that ever reads the config for other files, and it could be redesigned not to do so - by conservatively renaming only identifiers declared in the current file.

As you're aware Nathan but others aren't - this ability to request config for other files was a significant source of difficulty in D91029 - i.e. complexity of embedding clang-tidy.
Other features like inheritance, local vs global names, check option value priority, ConfigFileHandlers etc have contributed to a really complex model and greatly hinder the prospect of replacing ClangTidyOptionsProvider with something simpler, which is sorely needed IMO.

Currently AFAICS there's just *one* check that ever reads the config for other files, and it could be redesigned not to do so - by conservatively renaming only identifiers declared in the current file.

You've missed the point behind this. The purpose for this functionality is clang-tidy (usually) only gets ran on implementation files, This makes it hard to enforce specific checks on header files.
You're best bet is hoping that all the header files you include are using the same .clang-tidy config.
This gets even more challenging when using the run_clang_tidy script over a compilation database. If there isn't one single project wide configuration, You'll likely get conflicting errors and fixes.
Also there is only *one* check at the moment.