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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D75441#1901071 , @gribozavr2 wrote:

> > I think my preference is to go with isLanguageVersionSupported() and not 
> > use base classes.
>
> +1, I can see this working, but the introduction of new concepts does not 
> seem like it pulls its weight given the tiny amount of boilerplate that they 
> eliminate.


+1, I'm definitely against adding base classes for supported language versions. 
The increase in the API surface doesn't seem to pay off here.


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

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

[Nathan James via Phabricator via cfe-commits]

njames93 marked an inline comment as done.
njames93 added a comment.

I suppose the only other way would be having another virtual function like 
inside the helper classes

  virtual bool additionalLanguageConstraints(const LangOptions& LangOpts) const 
{ return true; }
  
  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
return LangOpts.CPlusPlus && additionalLanguageConstraints(LangOpts);
  }

However I'm not a huge fan of that extra boilerplate


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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

> I think my preference is to go with isLanguageVersionSupported() and not use 
> base classes.

+1, I can see this working, but the introduction of new concepts does not seem 
like it pulls its weight given the tiny amount of boilerplate that they 
eliminate.


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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 marked an inline comment as done.
njames93 added inline comments.



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;

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.


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

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

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, gribozavr2, alexfh, lebedev.ri, 
Eugene.Zelenko.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75441

Files:
  clang-tools-extra/clang-tidy/ClangTidyCheck.h


Index: clang-tools-extra/clang-tidy/ClangTidyCheck.h
===
--- clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/ClangTidyCheck.h
@@ -204,6 +204,39 @@
   const LangOptions &getLangOpts() const { return Context->getLangOpts(); }
 };
 
+/// Helper class for clang-tidy checks that only register when in `Cc mode.
+class CClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.C99;
+  }
+};
+
+/// Helper class for clang-tidy checks that only register when in `c++` mode.
+class CppClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.CPlusPlus;
+  }
+};
+
+/// Helper class for clang-tidy checks that only register when in `c++11` mode.
+class Cpp11ClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.CPlusPlus11;
+  }
+};
+
+/// Helper class for clang-tidy checks that only register when in `Objective-c`
+/// mode.
+class ObjCClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.ObjC;
+  }
+};
+
 } // namespace tidy
 } // namespace clang
 


Index: clang-tools-extra/clang-tidy/ClangTidyCheck.h
===
--- clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/ClangTidyCheck.h
@@ -204,6 +204,39 @@
   const LangOptions &getLangOpts() const { return Context->getLangOpts(); }
 };
 
+/// Helper class for clang-tidy checks that only register when in `Cc mode.
+class CClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.C99;
+  }
+};
+
+/// Helper class for clang-tidy checks that only register when in `c++` mode.
+class CppClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.CPlusPlus;
+  }
+};
+
+/// Helper class for clang-tidy checks that only register when in `c++11` mode.
+class Cpp11ClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.CPlusPlus11;
+  }
+};
+
+/// Helper class for clang-tidy checks that only register when in `Objective-c`
+/// mode.
+class ObjCClangTidyCheck : public ClangTidyCheck {
+  using ClangTidyCheck::ClangTidyCheck;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const final {
+return LangOpts.ObjC;
+  }
+};
+
 } // namespace tidy
 } // namespace clang
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits