Page MenuHomePhabricator

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

Authored by njames93 on Mar 2 2020, 2:06 AM.

Diff Detail

Event Timeline

njames93 created this revision.Mar 2 2020, 2:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 2:06 AM
njames93 marked an inline comment as done.Mar 2 2020, 2:08 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/ClangTidyCheck.h
224

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.

aaron.ballman added inline comments.Mar 2 2020, 7:20 AM
clang-tools-extra/clang-tidy/ClangTidyCheck.h
210

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.

211

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

224

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.

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.

njames93 marked an inline comment as done.Mar 2 2020, 7:54 AM

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

alexfh added a comment.Mar 3 2020, 2:52 AM

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.

njames93 abandoned this revision.Mar 3 2020, 3:49 AM