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