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.

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to