[PATCH] D75441: [clang-tidy] Add helper base check classes that only run on specific language versions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:210
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+    return LangOpts.C99;
----------------
The `final` means that one cannot compose these. e.g., you cannot have a C-only check that also checks for other language options like whether CUDA is enabled or not, which seems unfortunate.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:211
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+    return LangOpts.C99;
+  }
----------------
This is not the behavior I would expect from the class name -- this is a C99 check, not a C check. Use `!LangOpts.CPlusPlus` instead


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:224
+/// Helper class for clang-tidy checks that only register when in `c++11` mode.
+class Cpp11ClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
----------------
njames93 wrote:
> Given the amount of new stuff added in c++11 and how many checks require c++11 I thought having a separate c++11 mode class would be a good idea, c++14/17 probably not so much though.
I'm on the fence. It's certainly a plausible design, but I'm not certain I like having to go hunt in several places for "what are the conditions under which this check is enabled". Status quo is to always look in the `check()` function (or wherever the matcher is being registered), and the proposed changes (in other patches) is to move that to `isLanguageVersionSupported()` for a cleaner interface. I like that. But with this patch, now I have to either look at the base class or `isLanguageVersionSupported()` (and if we remove the `final`, then in both places as well).

I think my preference is to go with `isLanguageVersionSupported()` and not use base classes. However, if there is more per-language functionality expected to be added to these base classes, then maybe this design would carry more water for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75441