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

[AMS21]

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

[PiotrZSL]

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

447

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

198

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.

[carlosgalvezp]

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]

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?

[AMS21]

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]

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

[AMS21]

Bumping to ask for reviews

[PiotrZSL]

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

490

this is out of scope of this change, verify this

[AMS21]

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

[AMS21]

Removed unrelated change

[AMS21]

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]

+-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]

clang-tools-extra/docs/ReleaseNotes.rst

219

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

[AMS21]

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.

[Dinistro]

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.

[PiotrZSL]

In D148697#4420359, @Dinistro wrote:

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.

[Dinistro]

In D148697#4421268, @PiotrZSL wrote:

In D148697#4420359, @Dinistro wrote:

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.

[PiotrZSL]

In D148697#4421339, @Dinistro wrote:

In D148697#4421268, @PiotrZSL wrote:

In D148697#4420359, @Dinistro wrote:

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.

[PiotrZSL]

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

[Dinistro]

In D148697#4421420, @PiotrZSL wrote:

In D148697#4421339, @Dinistro wrote:

In D148697#4421268, @PiotrZSL wrote:

In D148697#4420359, @Dinistro wrote:

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]

In D148697#4421643, @PiotrZSL wrote:

@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

[PiotrZSL]

In D148697#4421739, @Dinistro wrote:

In D148697#4421643, @PiotrZSL wrote:

@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.

[nemanjai]

In D148697#4421739, @Dinistro wrote:

In D148697#4421643, @PiotrZSL wrote:

@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

[PiotrZSL]

In D148697#4422629, @nemanjai wrote:

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.

[nemanjai]

In D148697#4422642, @PiotrZSL wrote:

In D148697#4422629, @nemanjai wrote:

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.

[Dinistro]

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]

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

273

To conform with burprone-argument-comment.

Apologies for late review.