[clang-tidy] Add AllowFalseEvaluated flag to clang-tidy noexcept-move-constructor check #126897

Nechda · 2025-02-12T11:40:18Z

This pull request aims to add a new flag to customize the noexcept-move-constructor check in clang-tidy.

Problem. Now the code below triggers the warning:

#include <type_traits> /* External class */ struct NotMarkedNoexcept { int x; NotMarkedNoexcept() = default; // NOLINTNEXTLINE(performance-noexcept-move-constructor) NotMarkedNoexcept(NotMarkedNoexcept&& rhs); }; struct Complicated { static constexpr bool NOEXCEPT_MOVE = std::is_nothrow_move_constructible_v<NotMarkedNoexcept>; Complicated() = default; Complicated(Complicated&&) noexcept(NOEXCEPT_MOVE) = default; // Warning NotMarkedNoexcept inner; }; int main() {}

https://godbolt.org/z/E4E65jdhh

I had created a related issue: #126702

My proposal is to add an option to allow noexcept(expr), even if this expression can be folded to false. This would force users to mark their move constructors as noexcept, while at the same time, the check would not trigger if the move constructor is not actually noexcept.

github-actions · 2025-02-12T11:40:36Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-02-12T11:41:11Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Dmitry Nechitaev (Nechda)

Changes

This pull request aims to add a new flag to customize the noexcept-move-constructor check in clang-tidy.

Problem. Now the code below triggers the warning:

#include &lt;type_traits&gt; /* External class */ struct NotMarkedNoexcept { int x; NotMarkedNoexcept() = default; // NOLINTNEXTLINE(performance-noexcept-move-constructor) NotMarkedNoexcept(NotMarkedNoexcept&amp;&amp; rhs); }; struct Complicated { static constexpr bool NOEXCEPT_MOVE = std::is_nothrow_move_constructible_v&lt;NotMarkedNoexcept&gt;; Complicated() = default; Complicated(Complicated&amp;&amp;) noexcept(NOEXCEPT_MOVE) = default; // Warning NotMarkedNoexcept inner; }; int main() {}

https://godbolt.org/z/E4E65jdhh

I had created a related issue: #126702

My proposal is to add an option to allow noexcept(expr), even if this expression can be folded to false. This would force users to mark their move constructors as noexcept, while at the same time, the check would not trigger if the move constructor is not actually noexcept.

Full diff: https://github.com/llvm/llvm-project/pull/126897.diff

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp (+1-1)
(modified) clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h (+7-1)
(added) clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp (+63)

diff --git a/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp b/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp index 911cd1b533367..8371b6aafd8ba 100644 --- a/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp @@ -30,7 +30,7 @@ void NoexceptFunctionBaseCheck::check(const MatchFinder::MatchResult &Result) { const Expr *NoexceptExpr = ProtoType->getNoexceptExpr(); if (NoexceptExpr) { NoexceptExpr = NoexceptExpr->IgnoreImplicit(); - if (!isa<CXXBoolLiteralExpr>(NoexceptExpr)) + if (!isa<CXXBoolLiteralExpr>(NoexceptExpr) && !AllowFalseEvaluated) reportNoexceptEvaluatedToFalse(FuncDecl, NoexceptExpr); return; } diff --git a/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h b/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h index 4775219d7e439..cd1da1fbeca55 100644 --- a/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h +++ b/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h @@ -22,7 +22,8 @@ namespace clang::tidy::performance { class NoexceptFunctionBaseCheck : public ClangTidyCheck { public: NoexceptFunctionBaseCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : ClangTidyCheck(Name, Context) + , AllowFalseEvaluated(Options.getLocalOrGlobal("AllowFalseEvaluated", false)) {} bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { return LangOpts.CPlusPlus11 && LangOpts.CXXExceptions; @@ -33,6 +34,10 @@ class NoexceptFunctionBaseCheck : public ClangTidyCheck { return TK_IgnoreUnlessSpelledInSource; } + void storeOptions(ClangTidyOptions::OptionMap &Opts) override { + Options.store(Opts, "AllowFalseEvaluated", AllowFalseEvaluated); + } + protected: virtual DiagnosticBuilder reportMissingNoexcept(const FunctionDecl *FuncDecl) = 0; @@ -42,6 +47,7 @@ class NoexceptFunctionBaseCheck : public ClangTidyCheck { static constexpr StringRef BindFuncDeclName = "FuncDecl"; private: + bool AllowFalseEvaluated; utils::ExceptionSpecAnalyzer SpecAnalyzer; }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp new file mode 100644 index 0000000000000..3552eaa7c50ea --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp @@ -0,0 +1,63 @@ +// RUN: %check_clang_tidy %s performance-noexcept-move-constructor %t -- -- -fexceptions + +// RUN: %check_clang_tidy -check-suffix=CONFIG %s performance-noexcept-move-constructor,performance-noexcept-destructor %t -- \ +// RUN: -config="{CheckOptions: {performance-noexcept-move-constructor.AllowFalseEvaluated: true}}" \ +// RUN: -- -fexceptions + +namespace std +{ + template <typename T> + struct is_nothrow_move_constructible + { + static constexpr bool value = __is_nothrow_constructible(T, __add_rvalue_reference(T)); + }; +} // namespace std + +struct ThrowOnAnything { + ThrowOnAnything() noexcept(false); + ThrowOnAnything(ThrowOnAnything&&) noexcept(false); + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept + // CHECK-MESSAGES-CONFIG-NOT: :[[@LINE-2]]:3: warning: move constructors should be marked noexcept + ThrowOnAnything& operator=(ThrowOnAnything &&) noexcept(false); + ~ThrowOnAnything() noexcept(false); +}; + +struct C_1 { + static constexpr bool kFalse = false; + C_1(C_1&&) noexcept(kFalse) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + // CHECK-MESSAGES-CONFIG-NOT: :[[@LINE-2]]:25: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + + C_1 &operator=(C_1 &&) noexcept(kFalse); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: noexcept specifier on the move assignment operator evaluates to 'false' [performance-noexcept-move-constructor] + // CHECK-MESSAGES-CONFIG-NOT: :[[@LINE-2]]:37: warning: noexcept specifier on the move assignment operator evaluates to 'false' [performance-noexcept-move-constructor] +}; + +struct C_2 { + static constexpr bool kEval = std::is_nothrow_move_constructible<ThrowOnAnything>::value; + static_assert(!kEval); // kEval == false; + + C_2(C_2&&) noexcept(kEval) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + // CHECK-MESSAGES-CONFIG-NOT: :[[@LINE-2]]:25: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + + C_2 &operator=(C_2 &&) noexcept(kEval); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: noexcept specifier on the move assignment operator evaluates to 'false' [performance-noexcept-move-constructor] + // CHECK-MESSAGES-CONFIG-NOT: :[[@LINE-2]]:37: warning: noexcept specifier on the move assignment operator evaluates to 'false' [performance-noexcept-move-constructor] + + ThrowOnAnything field; +}; + +struct C_3 { + static constexpr bool kEval = std::is_nothrow_move_constructible<ThrowOnAnything>::value; + static_assert(!kEval); // kEval == false; + + C_3(C_3&&) noexcept(kEval) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + // CHECK-MESSAGES-CONFIG-NOT: :[[@LINE-2]]:25: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + + ~C_3() noexcept(kEval) = default; + // CHECK-MESSAGES-CONFIG: :[[@LINE-1]]:21: warning: noexcept specifier on the destructor evaluates to 'false' + + ThrowOnAnything field; +};

Nechda · 2025-02-12T11:58:35Z

Ask for review @PiotrZSL

EugeneZelenko · 2025-02-12T15:27:15Z

Please mention changes in Release Notes and check documentation.

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-move-constructor.rst

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-move-constructor.rst

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-move-constructor.rst

Nechda · 2025-02-17T10:51:55Z

I apologize for the spam in my emails. I am having some issues with the GitHub UI. The changes to the branch are only related to merging commits into one.

vbvictor · 2025-02-17T12:00:22Z

If you squash commits solely for merging into main - there is no need for it. They will be squashed automatically during merge.

Nechda · 2025-02-19T12:27:06Z

Ping

clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h

HerrCai0907 · 2025-02-24T05:27:10Z

clang-tools-extra/docs/ReleaseNotes.rst


+- Improved :doc:`performance-noexcept-move-constructor
+ <clang-tidy/checks/performance/noexcept-move-constructor>` check by adding
+ a new (off-by-default) option `AllowFalseEvaluated`, which allows marking


Normally, we add "default is xxx" at the end of option description.

Hm, these changes are related to the release notes, not the option description.

The check's documentation has already satisfied your change request. Changes

Usually we don't write default values of options in ReleaseNotes, you could remove it

github-actions · 2025-02-24T11:20:18Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Nechda · 2025-02-24T12:13:53Z

If you are happy with this change, please approve it and merge it. I do not have the necessary permissions to perform the merge operation.

Nechda · 2025-03-10T11:14:29Z

Ping

EugeneZelenko

Please rebase from main. There are some changes in Release Notes.

vbvictor

@Nechda, gentle ping, do you still wish to work on this PR? 21th LLVM release will be soon, If you wish to have your changes in the upcoming release please rebase on main and fix suggestions.

...tra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp

Nechda · 2025-06-04T10:44:32Z

Yes, this PR is still alive. I'll rebase it.

Nechda · 2025-06-06T07:47:32Z

I've rebased the branch, gentle pings to reviewers.

vbvictor

Since you de-facto added this option to performance-noexcept-destructor check too, please add it to performance-noexcept-destructor.rst docs (just copypaste), add to ReleaseNotes and add a separate test-file for noexcept destructors.

vbvictor · 2025-06-06T08:01:43Z

clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h

 public:
 NoexceptFunctionBaseCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ : ClangTidyCheck(Name, Context),


Please place definition in .cpp file.
We usually define only isLanguageVersionSupported and getCheckTraversalKind in headers

vbvictor · 2025-06-06T08:01:52Z

clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h

 return TK_IgnoreUnlessSpelledInSource;
 }

+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override {


Please place definition in .cpp file

vbvictor · 2025-06-06T08:05:09Z

clang-tools-extra/docs/ReleaseNotes.rst

 <clang-tidy/checks/readability/redundant-smartptr-get>` check by fixing
 some false positives involving smart pointers to arrays.

+- Improved :doc:`performance-noexcept-move-constructor


Please place this in alphabetical order (by check name).
Should be placed after performance-move-const-arg.

vbvictor · 2025-06-06T08:18:52Z

clang-tools-extra/docs/ReleaseNotes.rst


+- Improved :doc:`performance-noexcept-move-constructor
+ <clang-tidy/checks/performance/noexcept-move-constructor>` check by adding
+ a new (off-by-default) option `AllowFalseEvaluated`, which allows marking


Usually we don't write default values of options in ReleaseNotes, you could remove it

vbvictor · 2025-06-06T08:21:57Z

...tra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp

+ // CHECK-MESSAGES-CONFIG: :[[@LINE-1]]:21: warning: noexcept specifier on the destructor evaluates to 'false'
+
+ ThrowOnAnything field;
+};


Please add a test-case that basic functionality of the check is preserved - when there is no noexcept at all move-ctor is flagged

I'm sorry, I don't quite understand what test case you are referring to. It looks like the default behavior of this check is already covered by another test case. https://github.com/llvm/llvm-project/blob/a663e78a6eb6bbd20c0c74b3e6a78e24ee423544/clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp

After the test simplification, only one line should be displayed in the output if the option in the config is enabled.

If we assume that the check is a "black box" then after enabling your option it can, for example, stop emitting all messages. To make sure that we still flag C_1(C_1&&);, we need to add a small test-case for that.

To make it more clear, I can write something like

diff --git a/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp b/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp index 8371b6aafd8b..dd1ee023bde3 100644 --- a/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.cpp @@ -20,6 +20,9 @@ void NoexceptFunctionBaseCheck::check(const MatchFinder::MatchResult &Result) { const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(BindFuncDeclName); assert(FuncDecl); + if (AllowFalseEvaluated) + return; + if (SpecAnalyzer.analyze(FuncDecl) != utils::ExceptionSpecAnalyzer::State::Throwing) return;

and all the tests will still pass. But clearly I broke the whole check.

llvmbot added clang-tools-extra clang-tidy labels Feb 12, 2025

EugeneZelenko requested review from 5chmidti, HerrCai0907, PiotrZSL and carlosgalvezp February 12, 2025 15:27

EugeneZelenko reviewed Feb 12, 2025

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-move-constructor.rst Outdated Show resolved Hide resolved

EugeneZelenko reviewed Feb 12, 2025

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-move-constructor.rst Outdated Show resolved Hide resolved

EugeneZelenko reviewed Feb 12, 2025

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-move-constructor.rst Outdated Show resolved Hide resolved

vbvictor reviewed Feb 12, 2025

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

vbvictor reviewed Feb 12, 2025

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/performance/noexcept-move-constructor.rst Outdated Show resolved Hide resolved

Nechda force-pushed the noexcept-move-constructor-allow-false-evaluated branch 7 times, most recently from 6b9b6e4 to 4d5b0be Compare February 17, 2025 10:33

Nechda changed the title ~~Add AllowFalseEvaluated flag to clang-tidy noexcept-move-constructor check~~ [clang-tidy] Add AllowFalseEvaluated flag to clang-tidy noexcept-move-constructor check Feb 17, 2025

HerrCai0907 reviewed Feb 24, 2025

View reviewed changes

clang-tools-extra/clang-tidy/performance/NoexceptFunctionBaseCheck.h Outdated Show resolved Hide resolved

HerrCai0907 reviewed Feb 24, 2025

View reviewed changes

Nechda force-pushed the noexcept-move-constructor-allow-false-evaluated branch from 4d5b0be to c0eecd1 Compare February 24, 2025 11:16

Nechda requested a review from HerrCai0907 February 24, 2025 12:09

EugeneZelenko reviewed Mar 10, 2025

View reviewed changes

vbvictor reviewed Jun 4, 2025

View reviewed changes

...tra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp Outdated Show resolved Hide resolved

...tra/test/clang-tidy/checkers/performance/noexcept-move-constructor-allow-false-evaluated.cpp Outdated Show resolved Hide resolved

Nechda added 2 commits June 5, 2025 11:23

Add AllowFalseEvaluated to clang-tidy noexcept-move-constructor check

1c2407f

Simplify test

77ef35c

Nechda force-pushed the noexcept-move-constructor-allow-false-evaluated branch from 75e9e45 to 77ef35c Compare June 5, 2025 14:44

vbvictor requested changes Jun 6, 2025

View reviewed changes

[clang-tidy] Add AllowFalseEvaluated flag to clang-tidy noexcept-move-constructor check #126897

Are you sure you want to change the base?

[clang-tidy] Add AllowFalseEvaluated flag to clang-tidy noexcept-move-constructor check #126897

Uh oh!

Conversation

Nechda commented Feb 12, 2025

github-actions bot commented Feb 12, 2025

llvmbot commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Nechda commented Feb 12, 2025

EugeneZelenko commented Feb 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nechda commented Feb 17, 2025

vbvictor commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Nechda commented Feb 19, 2025

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Nechda commented Feb 24, 2025

Nechda commented Mar 10, 2025

EugeneZelenko left a comment

Choose a reason for hiding this comment

vbvictor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Nechda commented Jun 4, 2025

Nechda commented Jun 6, 2025

vbvictor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbvictor Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

5 participants

llvmbot commented Feb 12, 2025 •

edited

Loading

vbvictor commented Feb 17, 2025 •

edited

Loading

github-actions bot commented Feb 24, 2025 •

edited

Loading

vbvictor Jun 6, 2025 •

edited

Loading