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
Paths
| Differential D148697
[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
Also added cppcoreguidlines aliases for the 2 new checks as well as performance-noexcept-move-constructor This fixes llvm#62154
Diff Detail
Event TimelineComment Actions Few issues, first we should have single check for a single cppcoreguidelines rule. 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. 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...
Comment Actions 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:
Comment Actions 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 marked an inline comment as done. Comment ActionsSplit 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 Comment Actions On first glance, looks fine, but I didn't check it too deep. One thing that I see and do not like is duplication, maybe we could extract some "base check" from those 3 (in next step).
This revision is now accepted and ready to land.May 28 2023, 6:22 AM AMS21 marked 2 inline comments as done. Comment ActionsRename cppcoreguidelines-noexcept-move-constructor to cppcoreguidelines-noexcept-move-operations Comment Actions 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. Comment Actions +-LGTM Only one thing that could be still done here, is to extract some base class from those checks, and move common code there.
AMS21 marked 3 inline comments as done. Comment ActionsImplement the suggested changes
This revision was landed with ongoing or failed builds.Jun 13 2023, 12:05 PM Closed by commit rG474a2b9367ad: [clang-tidy] Add more checks for functions which should be noexcept (authored by AMS21, committed by PiotrZSL). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions 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. Comment Actions
You shouldn't because this library got dependency on clangTidyPerformanceModule. Comment Actions
I don't necessarily need to build clang-tidy, so it's fine for now. Comment Actions
I did a clean build with shared libraries enabled, everything is working. Comment Actions @Dinistro It compiles & links with clang 16 also (shared libs). So I'm unable to reproduce. Comment Actions
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. Comment Actions
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 Comment Actions
Still no issue on my side. Try cleaning up ccache and directory. Also by looking into code, everything looks fine, dependencies are correctly set. Comment Actions
It was also causing PPC bots to break. I fixed it in https://reviews.llvm.org/rGffd7a200fdfbd01ef296101647d2f2da91ddfd41 Comment Actions
O, thank you. Thats explain a lot. Because when I look into cmake, clangTidyPerformanceModule were already there because you already added it. Comment Actions
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. Comment Actions Sorry for the noise. I didn't pull in between the different runs and was still on the broken version. shafik added inline comments.
Revision Contents
Diff 518278 clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/clang-tidy/performance/CMakeLists.txt
clang-tools-extra/clang-tidy/performance/NoexceptDestructorCheck.h
clang-tools-extra/clang-tidy/performance/NoexceptDestructorCheck.cpp
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.h
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cppclang-tools-extra/clang-tidy/performance/NoexceptSwapCheck.h
clang-tools-extra/clang-tidy/performance/NoexceptSwapCheck.cpp
clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/LexerUtils.h
clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-destructor.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-move-constructor.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/noexcept-swap.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-destructor.rst
clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-swap.rst
clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-destructor.cpp
clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp
clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-swap.cpp
|
start with cxxDestructorDecl(unless(isImplicit()), ..., no need for functionDecl.
Also here we may have some issues with template dependent code.