Page MenuHomePhabricator

[clang-tidy] Change checks to use new isLanguageVersionSupported restriction
ClosedPublic

Authored by njames93 on Feb 28 2020, 4:18 AM.

Details

Summary

Modifies all checks that are language version dependent to use isLanguageVersionSupported

Diff Detail

Event Timeline

njames93 created this revision.Feb 28 2020, 4:18 AM

Looks good, but please move non-mechanical changes (potentially-semantic changes) into a separate patch.

clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.h
33

I think CPlusPlus17 implies CPlusPlus.

clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.h
31

(from a parent review) Not a critical thing, but I feel like it would be better to put isLanguageVersionSupported before register functions. That logical order makes more sense to a reader that is not familiar with the class.

If you end up making that change, it would be great if you could also apply it throughout this patch.

clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.h
39

Shouldn't this be a non-11 check?

clang-tools-extra/clang-tidy/modernize/DeprecatedIosBaseAliasesCheck.h
33

Also should be non-11 to preserve existing behavior?

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h
29

Ditto, non-11.

It seems however that this check indeed wants C++11. If you would like to make that change, please make pull it out into a separate patch.

clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.h
55

Ditto, please make semantic changes in a separate patch.

clang-tools-extra/clang-tidy/modernize/UseAutoCheck.h
28

Ditto.

clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.h
47

Ditto.

clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.h
49

Ditto.

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
37

Ditto.

njames93 updated this revision to Diff 247243.Feb 28 2020, 5:42 AM
njames93 marked 10 inline comments as done.
  • Removed functional changes for supported language
njames93 updated this revision to Diff 247527.Mar 1 2020, 4:05 PM
njames93 marked an inline comment as done.
  • Moved isLanguageVersionSupported to above register functions like in the base class
njames93 planned changes to this revision.Mar 2 2020, 2:10 AM

Made another pull request that could neaten this implementation up - https://reviews.llvm.org/D75441, will wait to see how that goes down before getting this pushed.

clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.h
33

This is saying it needs c++ but not c++17, so c++98->c++14 are Ok but not 17 or 20. Besides this is the current behaviour in the cpp file

njames93 requested review of this revision.Mar 3 2020, 5:30 AM

Support for the previous point didnt go too well so will stick with the current implementation

gribozavr2 accepted this revision.Mar 3 2020, 8:03 AM
gribozavr2 added inline comments.
clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.h
33

Sorry, I misread and didn't notice the exclamation mark. Your edit makes sense to me.

I commented because I was comparing with the .cpp file which was not checking CPlusPlus:

void DefaultOperatorNewAlignmentCheck::registerMatchers(MatchFinder *Finder) {
  // Check not applicable in C++17 (or newer).
  if (getLangOpts().CPlusPlus17)
    return;

So there is a tiny change here: previously, this check was being registered in non-C++ language modes, now it is not. I think your change to only enable in C++ modes is good, but again I think it would be best done in a separate patch.

This revision is now accepted and ready to land.Mar 3 2020, 8:03 AM
njames93 updated this revision to Diff 247918.Mar 3 2020, 8:43 AM
  • Remove semantic changes from DefaultOperatorNew
This revision was automatically updated to reflect the committed changes.