[clang-tidy] Extend readability-else-after-return to remove else after calls to [[noreturn]] functions#185202
Open
berkaysahiin wants to merge 2 commits intollvm:mainfrom
Open
[clang-tidy] Extend readability-else-after-return to remove else after calls to [[noreturn]] functions#185202berkaysahiin wants to merge 2 commits intollvm:mainfrom
berkaysahiin wants to merge 2 commits intollvm:mainfrom
Conversation
…r calls to [[noreturn]] functions
Member
|
@llvm/pr-subscribers-clang-tidy Author: Berkay Sahin (berkaysahiin) ChangesCloses #184930 Full diff: https://github.com/llvm/llvm-project/pull/185202.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
index 7e93d619e2a9f..4160f0fcf1dc1 100644
--- a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -39,6 +39,23 @@ class PPConditionalCollector : public PPCallbacks {
const SourceManager &SM;
};
+bool isNoReturnStmt(const Stmt &Stmt) {
+ const auto *Call = dyn_cast<CallExpr>(&Stmt);
+ if (!Call)
+ return false;
+
+ const FunctionDecl *Func = Call->getDirectCallee();
+ if (!Func)
+ return false;
+
+ return Func->isNoReturn();
+}
+
+AST_MATCHER(Stmt, isNoReturnStmt) {
+ const Stmt &S = Node;
+ return isNoReturnStmt(S);
+}
+
AST_MATCHER_P(Stmt, stripLabelLikeStatements,
ast_matchers::internal::Matcher<Stmt>, InnerMatcher) {
const Stmt *S = Node.stripLabelLikeStatements();
@@ -174,7 +191,8 @@ void ElseAfterReturnCheck::registerPPCallbacks(const SourceManager &SM,
void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
const auto InterruptsControlFlow = stmt(anyOf(
returnStmt().bind(InterruptingStr), continueStmt().bind(InterruptingStr),
- breakStmt().bind(InterruptingStr), cxxThrowExpr().bind(InterruptingStr)));
+ breakStmt().bind(InterruptingStr), cxxThrowExpr().bind(InterruptingStr),
+ stmt(isNoReturnStmt()).bind(InterruptingStr)));
const auto IfWithInterruptingThenElse =
ifStmt(unless(isConstexpr()), unless(isConsteval()),
@@ -237,6 +255,8 @@ static StringRef getControlFlowString(const Stmt &Stmt) {
return "break";
if (isa<CXXThrowExpr>(Stmt))
return "throw";
+ if (isNoReturnStmt(Stmt))
+ return "noreturn";
llvm_unreachable("Unknown control flow interrupter");
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b0b4cd646c3bd..33437d2004079 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -295,8 +295,13 @@ Changes in existing checks
when a member expression has a non-identifier name.
- Improved :doc:`readability-else-after-return
- <clang-tidy/checks/readability/else-after-return>` check by fixing missed
- diagnostics when ``if`` statements appear in unbraced ``switch`` case labels.
+ <clang-tidy/checks/readability/else-after-return>` check:
+
+ - Fixed missed diagnostics when ``if`` statements appear in unbraced
+ ``switch`` case labels.
+
+ - Diagnose and remove redundant ``else`` branches after calls to
+ ``[[noreturn]]`` functions.
- Improved :doc:`readability-enum-initial-value
<clang-tidy/checks/readability/enum-initial-value>` check: the warning message
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp
index e987687a764cd..f0788dbf3221b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp
@@ -438,3 +438,15 @@ void testLabels(bool b) {
f(0);
}
}
+
+[[noreturn]] void noReturn();
+
+void testNoReturn() {
+ if (true) {
+ noReturn();
+ } else { // comment-28
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'noreturn'
+ // CHECK-FIXES: {{^}} } // comment-28
+ f(0);
+ }
+}
|
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: Berkay Sahin (berkaysahiin) ChangesCloses #184930 Full diff: https://github.com/llvm/llvm-project/pull/185202.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
index 7e93d619e2a9f..4160f0fcf1dc1 100644
--- a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -39,6 +39,23 @@ class PPConditionalCollector : public PPCallbacks {
const SourceManager &SM;
};
+bool isNoReturnStmt(const Stmt &Stmt) {
+ const auto *Call = dyn_cast<CallExpr>(&Stmt);
+ if (!Call)
+ return false;
+
+ const FunctionDecl *Func = Call->getDirectCallee();
+ if (!Func)
+ return false;
+
+ return Func->isNoReturn();
+}
+
+AST_MATCHER(Stmt, isNoReturnStmt) {
+ const Stmt &S = Node;
+ return isNoReturnStmt(S);
+}
+
AST_MATCHER_P(Stmt, stripLabelLikeStatements,
ast_matchers::internal::Matcher<Stmt>, InnerMatcher) {
const Stmt *S = Node.stripLabelLikeStatements();
@@ -174,7 +191,8 @@ void ElseAfterReturnCheck::registerPPCallbacks(const SourceManager &SM,
void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
const auto InterruptsControlFlow = stmt(anyOf(
returnStmt().bind(InterruptingStr), continueStmt().bind(InterruptingStr),
- breakStmt().bind(InterruptingStr), cxxThrowExpr().bind(InterruptingStr)));
+ breakStmt().bind(InterruptingStr), cxxThrowExpr().bind(InterruptingStr),
+ stmt(isNoReturnStmt()).bind(InterruptingStr)));
const auto IfWithInterruptingThenElse =
ifStmt(unless(isConstexpr()), unless(isConsteval()),
@@ -237,6 +255,8 @@ static StringRef getControlFlowString(const Stmt &Stmt) {
return "break";
if (isa<CXXThrowExpr>(Stmt))
return "throw";
+ if (isNoReturnStmt(Stmt))
+ return "noreturn";
llvm_unreachable("Unknown control flow interrupter");
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b0b4cd646c3bd..33437d2004079 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -295,8 +295,13 @@ Changes in existing checks
when a member expression has a non-identifier name.
- Improved :doc:`readability-else-after-return
- <clang-tidy/checks/readability/else-after-return>` check by fixing missed
- diagnostics when ``if`` statements appear in unbraced ``switch`` case labels.
+ <clang-tidy/checks/readability/else-after-return>` check:
+
+ - Fixed missed diagnostics when ``if`` statements appear in unbraced
+ ``switch`` case labels.
+
+ - Diagnose and remove redundant ``else`` branches after calls to
+ ``[[noreturn]]`` functions.
- Improved :doc:`readability-enum-initial-value
<clang-tidy/checks/readability/enum-initial-value>` check: the warning message
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp
index e987687a764cd..f0788dbf3221b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp
@@ -438,3 +438,15 @@ void testLabels(bool b) {
f(0);
}
}
+
+[[noreturn]] void noReturn();
+
+void testNoReturn() {
+ if (true) {
+ noReturn();
+ } else { // comment-28
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'noreturn'
+ // CHECK-FIXES: {{^}} } // comment-28
+ f(0);
+ }
+}
|
|
✅ With the latest revision this PR passed the C/C++ code linter. |
localspook
reviewed
Mar 8, 2026
Contributor
localspook
left a comment
localspook
left a comment
There was a problem hiding this comment.
This check is getting a lot of love this release ^_^
| // CHECK-FIXES: {{^}} } // comment-28 | ||
| f(0); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Can we add a test with a member function marked [[noreturn]]?
| returnStmt().bind(InterruptingStr), continueStmt().bind(InterruptingStr), | ||
| breakStmt().bind(InterruptingStr), cxxThrowExpr().bind(InterruptingStr))); | ||
| breakStmt().bind(InterruptingStr), cxxThrowExpr().bind(InterruptingStr), | ||
| stmt(isNoReturnStmt()).bind(InterruptingStr))); |
Contributor
There was a problem hiding this comment.
I think this could just be written as:
Suggested change
| stmt(isNoReturnStmt()).bind(InterruptingStr))); | |
| callExpr(callee(functionDecl(isNoReturn()))).bind(InterruptingStr))); |
with no need for the custom matchers.
Comment on lines
+258
to
+259
| if (isNoReturnStmt(Stmt)) | ||
| return "noreturn"; |
Contributor
There was a problem hiding this comment.
The message
do not use 'else' after 'noreturn'seems a bit opaque. I would suggest something like
do not use 'else' after calling a function that doesn't returnYou'll have to remove the single quotes around %0 here:
and move them into
getControlFlowString.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #184930