This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add more checks for functions which should be noexcept
ClosedPublic

Authored by AMS21 on Apr 19 2023, 3:00 AM.

Details

Summary

Added new checks

  • performance-noexcept-destructor
  • performance-noexcept-swap

Also added cppcoreguidlines aliases for the 2 new checks as well as performance-noexcept-move-constructor

This fixes llvm#62154

Diff Detail

Event Timeline

AMS21 created this revision.Apr 19 2023, 3:00 AM
AMS21 requested review of this revision.Apr 19 2023, 3:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 3:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Few issues, first we should have single check for a single cppcoreguidelines rule.
And due to that I'm not fan of merging these things, because for example If I would enable this check in my project I woudn't like to enforce noexcept default constructor or destructor. Simply there are other checks for destructors .

This is why better would be to rename this check into performance-noexcept-move-special-functions, and add other checks for swap, hash, default constructor.
Where one for default constructor could be cpp-core-guidelines only check, and swap, hash could have some performance code.
For example in libcxx if hash function is noexcept, then hash value is not cached in container, but when is not noexcept then it's cached. But thats only libcxx.
Also things like compare operators for me should be noexcept...

If you want to keep single check, I'm fine with it, but consider adding option to disable some parts of it (mainly default constructors), and consider including (not necessary in this patch, all compare operators).

Give it a thing...

clang-tools-extra/docs/ReleaseNotes.rst
371

Add release notes info about check rename, best as separate entry.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-special-functions.rst
10 ↗(On Diff #514879)

Add link to implemented cpp core guidelines rules.
Add reference
C.66: Make move operations noexcept
C.83: For value-like types, consider providing a noexcept swap function.
Same goes to operator == (C.86 rule)
And hash C.89 rule

clang-tools-extra/docs/clang-tidy/checks/list.rst
195

This list os for checks,
Add alias under .. csv-table:: Aliases..

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-special-functions.rst
7 ↗(On Diff #514879)

user defined default constructor does not need to be noexcept.
There is rule C.44: Prefer default constructors to be simple and non-throwing but that.s just a guideline.

8 ↗(On Diff #514879)

add entry here why destructors and swap functions should be also noexcept.

First of all thank you for the contribution! I had just a quick look so here's some very preliminary comments, will have more time to deeply review during the weekend:

  • Unfortunately we cannot simply rename a check like this, since it breaks backwards compatibility. We need a period of 2 major releases where both the new check and the old check (marked as deprecated) co-exist, so people have enough time to port over. Only after those 2 releases can the old check be fully removed.
  • Contrary to my comment on Github, I no longer think this should be a cppcoreguideline check, as it does not implement rule F.6. The enforcement required is to flag all functions (not just the special ones) that cannot throw and mark them as noexcept. That's a huge difference with a larger scope and impact in a codebase. I personally think that last bit is impractical in a real-world project, it would add too much noise.
  • This check is also used as an alias to another guideline - HiCPP Rule 12.5.4, which strictly only covers move operators. Guideline checks (HiCPP, CppCoreGuidelines) are stricter than the regular checks, and should only cover what they intend to cover - no more, no less.
  • Special member functions are also copy constructors & assignments, yet those don't seem to be part of the rule?
carlosgalvezp added inline comments.Apr 19 2023, 8:59 AM
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-special-functions.rst
10 ↗(On Diff #514879)

Since there are different rules for different functions it's probably easier to maintain if we have 1 check per rule. Having a large check doing multiple things and multiple reasons for change can lead to more bugs, churn, renames, etc. What do you think?

Well first of all thanks for the feedback. I agree not renaming a check which is already used and splitting each special case into it's own check seems like the easier and better way to go.

AMS21 updated this revision to Diff 518278.Apr 30 2023, 12:50 AM
AMS21 marked an inline comment as done.

Split the changes into 2 new checks and added cppcoreguidelines aliases for all of them.

AMS21 retitled this revision from [clang-tidy] Handle more cases of functions which should always be noexcept to [clang-tidy] Add more checks for functions which should be noexcept.Apr 30 2023, 12:56 AM
AMS21 edited the summary of this revision. (Show Details)
AMS21 marked an inline comment as done.Apr 30 2023, 12:58 AM
AMS21 added a comment.May 28 2023, 5:57 AM

Bumping to ask for reviews

PiotrZSL accepted this revision.May 28 2023, 6:22 AM

On first glance, looks fine, but I didn't check it too deep.
Leave it open for 1-2 weeks. I will try to check it more deeply.

One thing that I see and do not like is duplication, maybe we could extract some "base check" from those 3 (in next step).

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-move-constructor.rst
5 ↗(On Diff #518278)

maybe then it should be called cppcoreguidelines-noexcept-move-operations

clang-tools-extra/docs/clang-tidy/checks/list.rst
486

this is out of scope of this change, verify this

This revision is now accepted and ready to land.May 28 2023, 6:22 AM
AMS21 updated this revision to Diff 526326.May 28 2023, 7:02 AM
AMS21 marked 2 inline comments as done.

Rename cppcoreguidelines-noexcept-move-constructor to cppcoreguidelines-noexcept-move-operations

AMS21 updated this revision to Diff 526327.May 28 2023, 7:04 AM

Removed unrelated change

AMS21 added a comment.Jun 13 2023, 3:44 AM

Patch is now open for 2+ weeks. If there are no more reviews, I would kindly ask for someone to push this on my behalf.

PiotrZSL accepted this revision.Jun 13 2023, 9:43 AM

+-LGTM

Only one thing that could be still done here, is to extract some base class from those checks, and move common code there.
I will commit this tomorrow, so if you decide to change anything you can still do that, if not you can always do that in separate patch.

clang-tools-extra/clang-tidy/performance/NoexceptDestructorCheck.cpp
20–24

start with cxxDestructorDecl(unless(isImplicit()), ..., no need for functionDecl.
Also here we may have some issues with template dependent code.

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp
22

I think we could use TK_IgnoreUnlessSpelledInSource instead of all those isImplicit

clang-tools-extra/clang-tidy/performance/NoexceptSwapCheck.cpp
22

i dont think that swap functions can be deleted

PiotrZSL added inline comments.Jun 13 2023, 10:20 AM
clang-tools-extra/docs/ReleaseNotes.rst
193

you got spelling wrong in these, its clang-tidy, not tify

AMS21 updated this revision to Diff 531005.Jun 13 2023, 11:23 AM
AMS21 marked 3 inline comments as done.

Implement the suggested changes

clang-tools-extra/clang-tidy/performance/NoexceptSwapCheck.cpp
22

All functions can be deleted. See cppreference

void swap(int, int) = delete;

Should be valid C++ and not give a no-noexcept warning, so we need to exclude it.

PiotrZSL accepted this revision.Jun 13 2023, 11:57 AM
This revision was landed with ongoing or failed builds.Jun 13 2023, 12:05 PM
This revision was automatically updated to reflect the committed changes.

I'm getting a linking error by some of the files changed in this revision.

ld.lld: error: undefined symbol: vtable for clang::tidy::performance::NoexceptDestructorCheck
>>> referenced by NoexceptDestructorCheck.h:26 (/home/christianu/repos/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/../performance/NoexceptDestructorCheck.h:26)
>>>               tools/clang/tools/extra/clang-tidy/cppcoreguidelines/CMakeFiles/obj.clangTidyCppCoreGuidelinesModule.dir/CppCoreGuidelinesTidyModule.cpp.o:(std::enable_if<is_invocable_r_v<std::uni
que_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::tidy::ClangTidyCheck>>, void clang::tidy::ClangTidyCheckFactories::registerCheck<clang::tidy::performance::NoexceptDestructorCheck>(ll
vm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef, clang::tidy::ClangTidyContext*>, std::unique_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::
tidy::ClangTidyCheck>>>::type std::__invoke_r<std::unique_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::tidy::ClangTidyCheck>>, void clang::tidy::ClangTidyCheckFactories::registerCheck
<clang::tidy::performance::NoexceptDestructorCheck>(llvm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef, clang::tidy::ClangTidyContext*>(void clang::tidy::C
langTidyCheckFactories::registerCheck<clang::tidy::performance::NoexceptDestructorCheck>(llvm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef&&, clang::tidy:
:ClangTidyContext*&&))
>>> the vtable symbol may be undefined because the class is missing its key function (see https://lld.llvm.org/missingkeyfunction)

ld.lld: error: undefined symbol: vtable for clang::tidy::performance::NoexceptMoveConstructorCheck
>>> referenced by NoexceptMoveConstructorCheck.h:30 (/home/christianu/repos/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/../performance/NoexceptMoveConstructorCheck.h:30)
>>>               tools/clang/tools/extra/clang-tidy/cppcoreguidelines/CMakeFiles/obj.clangTidyCppCoreGuidelinesModule.dir/CppCoreGuidelinesTidyModule.cpp.o:(std::enable_if<is_invocable_r_v<std::uni
que_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::tidy::ClangTidyCheck>>, void clang::tidy::ClangTidyCheckFactories::registerCheck<clang::tidy::performance::NoexceptMoveConstructorChec
k>(llvm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef, clang::tidy::ClangTidyContext*>, std::unique_ptr<clang::tidy::ClangTidyCheck, std::default_delete<cl
ang::tidy::ClangTidyCheck>>>::type std::__invoke_r<std::unique_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::tidy::ClangTidyCheck>>, void clang::tidy::ClangTidyCheckFactories::register
Check<clang::tidy::performance::NoexceptMoveConstructorCheck>(llvm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef, clang::tidy::ClangTidyContext*>(void clan
g::tidy::ClangTidyCheckFactories::registerCheck<clang::tidy::performance::NoexceptMoveConstructorCheck>(llvm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef&
&, clang::tidy::ClangTidyContext*&&))
>>> the vtable symbol may be undefined because the class is missing its key function (see https://lld.llvm.org/missingkeyfunction)

ld.lld: error: undefined symbol: vtable for clang::tidy::performance::NoexceptSwapCheck
>>> referenced by NoexceptSwapCheck.h:26 (/home/christianu/repos/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/../performance/NoexceptSwapCheck.h:26)
>>>               tools/clang/tools/extra/clang-tidy/cppcoreguidelines/CMakeFiles/obj.clangTidyCppCoreGuidelinesModule.dir/CppCoreGuidelinesTidyModule.cpp.o:(std::enable_if<is_invocable_r_v<std::uni
que_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::tidy::ClangTidyCheck>>, void clang::tidy::ClangTidyCheckFactories::registerCheck<clang::tidy::performance::NoexceptSwapCheck>(llvm::St
ringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef, clang::tidy::ClangTidyContext*>, std::unique_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::tidy::
ClangTidyCheck>>>::type std::__invoke_r<std::unique_ptr<clang::tidy::ClangTidyCheck, std::default_delete<clang::tidy::ClangTidyCheck>>, void clang::tidy::ClangTidyCheckFactories::registerCheck<clang
::tidy::performance::NoexceptSwapCheck>(llvm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef, clang::tidy::ClangTidyContext*>(void clang::tidy::ClangTidyChec
kFactories::registerCheck<clang::tidy::performance::NoexceptSwapCheck>(llvm::StringRef)::'lambda'(llvm::StringRef, clang::tidy::ClangTidyContext*)&, llvm::StringRef&&, clang::tidy::ClangTidyContext*
&&))
>>> the vtable symbol may be undefined because the class is missing its key function (see https://lld.llvm.org/missingkeyfunction)
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

I'm building with shared libraries on.

I'm getting a linking error by some of the files changed in this revision.

You shouldn't because this library got dependency on clangTidyPerformanceModule.
Is this a blocker for you, or you can live with this for 1 day ? Until I verify this.

I'm getting a linking error by some of the files changed in this revision.

You shouldn't because this library got dependency on clangTidyPerformanceModule.
Is this a blocker for you, or you can live with this for 1 day ? Until I verify this.

I don't necessarily need to build clang-tidy, so it's fine for now.

I'm getting a linking error by some of the files changed in this revision.

You shouldn't because this library got dependency on clangTidyPerformanceModule.
Is this a blocker for you, or you can live with this for 1 day ? Until I verify this.

I don't necessarily need to build clang-tidy, so it's fine for now.

I did a clean build with shared libraries enabled, everything is working.
I do not get those issues. Try a clean-build. I will try to check build with clang-16.

@Dinistro It compiles & links with clang 16 also (shared libs). So I'm unable to reproduce.

I'm getting a linking error by some of the files changed in this revision.

You shouldn't because this library got dependency on clangTidyPerformanceModule.
Is this a blocker for you, or you can live with this for 1 day ? Until I verify this.

I don't necessarily need to build clang-tidy, so it's fine for now.

I did a clean build with shared libraries enabled, everything is working.
I do not get those issues. Try a clean-build. I will try to check build with clang-16.

I tried with a clean build, and I get the exact same issues. Might be caused by the lld and clang-16 combination, not sure.

@Dinistro It compiles & links with clang 16 also (shared libs). So I'm unable to reproduce.

I used the following command to build:

cmake -S/home/christianu/repos/llvm-project/llvm -B/home/christianu/repos/llvm-project/build-clang-tidy -GNinja -DLLVM_ENABLE_LLD=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_ASSERTIONS=ON -DBUILD_SHARED_LIBS=ON -DLLVM_INCLUDE_UTILS=ON -DLLVM_INSTALL_UTILS=ON -DLLVM_BUILD_EXAMPLES=OFF -DLLVM_CCACHE_BUILD=ON -DLLVM_OPTIMIZED_TABLEGEN=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_RUNTIMES=openmp

@Dinistro It compiles & links with clang 16 also (shared libs). So I'm unable to reproduce.

I used the following command to build:

cmake -S/home/christianu/repos/llvm-project/llvm -B/home/christianu/repos/llvm-project/build-clang-tidy -GNinja -DLLVM_ENABLE_LLD=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_ASSERTIONS=ON -DBUILD_SHARED_LIBS=ON -DLLVM_INCLUDE_UTILS=ON -DLLVM_INSTALL_UTILS=ON -DLLVM_BUILD_EXAMPLES=OFF -DLLVM_CCACHE_BUILD=ON -DLLVM_OPTIMIZED_TABLEGEN=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_RUNTIMES=openmp

Still no issue on my side. Try cleaning up ccache and directory. Also by looking into code, everything looks fine, dependencies are correctly set.

@Dinistro It compiles & links with clang 16 also (shared libs). So I'm unable to reproduce.

I used the following command to build:

cmake -S/home/christianu/repos/llvm-project/llvm -B/home/christianu/repos/llvm-project/build-clang-tidy -GNinja -DLLVM_ENABLE_LLD=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_ASSERTIONS=ON -DBUILD_SHARED_LIBS=ON -DLLVM_INCLUDE_UTILS=ON -DLLVM_INSTALL_UTILS=ON -DLLVM_BUILD_EXAMPLES=OFF -DLLVM_CCACHE_BUILD=ON -DLLVM_OPTIMIZED_TABLEGEN=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_RUNTIMES=openmp

It was also causing PPC bots to break. I fixed it in https://reviews.llvm.org/rGffd7a200fdfbd01ef296101647d2f2da91ddfd41

It was also causing PPC bots to break. I fixed it in https://reviews.llvm.org/rGffd7a200fdfbd01ef296101647d2f2da91ddfd41

O, thank you. Thats explain a lot. Because when I look into cmake, clangTidyPerformanceModule were already there because you already added it.

It was also causing PPC bots to break. I fixed it in https://reviews.llvm.org/rGffd7a200fdfbd01ef296101647d2f2da91ddfd41

O, thank you. Thats explain a lot. Because when I look into cmake, clangTidyPerformanceModule were already there because you already added it.

Yeah, sorry. I saw that Phabricator automatically added the mention into the Diffusion link but I forgot that it won't add it to the Differential and that I need to add it manually.

Sorry for the noise. I didn't pull in between the different runs and was still on the broken version.
Thanks for fixing this issue :)

shafik added a subscriber: shafik.Jun 21 2023, 8:11 PM
shafik added inline comments.
clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
273

To conform with burprone-argument-comment.

Apologies for late review.