This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Refactor common code from the Noexcept*Checks into `NoexceptFunctionCheck`
ClosedPublic

Authored by AMS21 on Jun 17 2023, 5:20 AM.

Diff Detail

Event Timeline

AMS21 created this revision.Jun 17 2023, 5:20 AM
AMS21 requested review of this revision.Jun 17 2023, 5:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2023, 5:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
AMS21 edited the summary of this revision. (Show Details)Jun 17 2023, 5:25 AM

Overall good, my main concern is base class name & location.

clang-tools-extra/clang-tidy/performance/NoexceptDestructorCheck.h
23

Consider putting "Base" into a name, so that it woudn't be considered a full check, and it can be in this module, no need to put it into utils

24–26

we got c++17, so delegate constructor.
using utils::NoexceptFunctionCheck::NoexceptFunctionCheck;

27–28

consider moving this into base class

clang-tools-extra/clang-tidy/utils/NoexceptFunctionCheck.h
34–38 ↗(On Diff #532388)

you can move those into protected.

AMS21 updated this revision to Diff 532394.Jun 17 2023, 6:26 AM
AMS21 marked 3 inline comments as done.

Implement suggested changes

PiotrZSL accepted this revision.Jun 17 2023, 8:40 AM

The is compiler error in CI "Cannot find source file: BaseNoexceptFunctionCheck.cpp".
Consider naming it NoexceptFunctionBase.cpp or NoexceptFunctionBaseCheck.cpp unless there are already other .cpp files in some other modules that start with Base, in such case its fine.
Main reason for this is to somehow show others that this file is not an full check, but more a base for some checks.

clang-tools-extra/clang-tidy/performance/NoexceptSwapCheck.h
31

you can put those all as private here in derived class.

This revision is now accepted and ready to land.Jun 17 2023, 8:40 AM
AMS21 updated this revision to Diff 532414.Jun 17 2023, 10:27 AM
AMS21 marked an inline comment as done.

So somehow I didn't add the actual BaseNoexceptFunctionCheck files, thats why the build failed.

Also implemented the two new suggestions.

PiotrZSL accepted this revision.Jun 17 2023, 10:58 AM
AMS21 added a comment.Jun 17 2023, 7:40 PM

If there are no more problems, I would kindly ask for someone to push this no my behalf :)