[PATCH] D141000: [clang-tidy] Introduce HeaderFileExtensions option

Thu Jan 5 10:53:35 PST 2023

carlosgalvezp added a comment.

Thanks for the feedback!

> Can you explain the reasoning of why this approach is better than current approach where checks can use global options(`Options.getLocalOrGlobal("HeaderFileExtensions", utils::defaultHeaderFileExtensions())`) to access the same information?

Nice, I wasn't aware of the  `getLocalOrGlobal` function. It's unclear to me why some arguments are available via both command line and config, and others only via config. If there is no use case for CLI interface I agree the global option is sufficient, but I would like to properly document it. There's for example the option `User: user` that is undocumented. Perhaps I can restructure a bit the section `Configuration files`?

> Some checks also have the global option `ImplementationFileExtensions`, what's the proposal with that. Should checks that use this functionality carry on like that, use a negative match of this proposed option or should we also add that option to the `ClangTidyOptions`.

For consistency I believe it makes sense to add it as well as a global option.

> What is the migration plan for code-bases that currently use the old check-based option? We can't break their existing configs, likewise we have to be careful of projects with checked-in .clang-tidy files that make use of this new option when maintainers may still have an older version of clang-tidy.

We would follow a deprecation strategy. For example, we document in every check that the current options are deprecated, and users should instead use the global config. The local config can be removed after 2 clang releases (that would mean clang 18 here). Using `getLocalOrGlobal` should simplify the logic of supporting the transition I believe.

> Finally, Why would you make the option a semicolon delimited string. The only reason that's used for the current `CheckOptions` is there isn't currently a better alternative, For `ClangTidyOptions` we could set the type to be an `Optional<std::vector<std::string>>`. This would be more natural when writing the yaml file.

Absolutely! The current format is very strange to me, but I kept it to keep the patch as minimal as possible. A vector of strings looks great.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141000