[clang-tidy] improve messages when auto-fix does not work #96917
Conversation
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
|
@llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) ChangesFixes: #93157 Full diff: https://github.com/llvm/llvm-project/pull/96917.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index fd4730d9c8b9c..499c88ef5d4e4 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -9,6 +9,7 @@
#include "SimplifyBooleanExprCheck.h"
#include "clang/AST/Expr.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/DiagnosticIDs.h"
#include "clang/Lex/Lexer.h"
#include "llvm/Support/SaveAndRestore.h"
@@ -702,7 +703,8 @@ bool SimplifyBooleanExprCheck::canBeBypassed(const Stmt *S) const {
return IgnoreMacros && S->getBeginLoc().isMacroID();
}
-void SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
+/// @brief return true when replacement created.
+bool SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
SourceLocation Loc,
StringRef Description,
SourceRange ReplacementRange,
@@ -712,8 +714,10 @@ void SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
Context.getSourceManager(), getLangOpts());
DiagnosticBuilder Diag = diag(Loc, Description);
- if (!containsDiscardedTokens(Context, CharRange))
+ const bool HasReplacement = !containsDiscardedTokens(Context, CharRange);
+ if (HasReplacement)
Diag << FixItHint::CreateReplacement(CharRange, Replacement);
+ return HasReplacement;
}
void SimplifyBooleanExprCheck::replaceWithThenStatement(
@@ -751,8 +755,18 @@ void SimplifyBooleanExprCheck::replaceWithReturnCondition(
replacementExpression(Context, Negated, If->getCond());
std::string Replacement = ("return " + Condition + Terminator).str();
SourceLocation Start = BoolLiteral->getBeginLoc();
- issueDiag(Context, Start, SimplifyConditionalReturnDiagnostic,
- If->getSourceRange(), Replacement);
+
+ const bool HasReplacement =
+ issueDiag(Context, Start, SimplifyConditionalReturnDiagnostic,
+ If->getSourceRange(), Replacement);
+
+ if (!HasReplacement) {
+ const SourceRange ConditionRange = If->getCond()->getSourceRange();
+ if (ConditionRange.isValid())
+ diag(ConditionRange.getBegin(), "conditions that can be simplified",
+ DiagnosticIDs::Note)
+ << ConditionRange;
+ }
}
void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
@@ -760,9 +774,23 @@ void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
const IfStmt *If, const Expr *ThenReturn) {
const std::string Replacement =
"return " + replacementExpression(Context, Negated, If->getCond());
- issueDiag(Context, ThenReturn->getBeginLoc(),
- SimplifyConditionalReturnDiagnostic,
- SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+
+ const bool HasReplacement = issueDiag(
+ Context, ThenReturn->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+ SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+
+ if (!HasReplacement) {
+ const SourceRange ConditionRange = If->getCond()->getSourceRange();
+ if (ConditionRange.isValid())
+ diag(ConditionRange.getBegin(), "conditions that can be simplified",
+ DiagnosticIDs::Note)
+ << ConditionRange;
+ const SourceRange ReturnRange = Ret->getSourceRange();
+ if (ReturnRange.isValid())
+ diag(ReturnRange.getBegin(), "return statement that can be simplified",
+ DiagnosticIDs::Note)
+ << ReturnRange;
+ }
}
void SimplifyBooleanExprCheck::replaceWithAssignment(const ASTContext &Context,
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
index 63c3caa01e01a..2ea6968798408 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -60,7 +60,7 @@ class SimplifyBooleanExprCheck : public ClangTidyCheck {
const BinaryOperator *Inner, bool TryOfferFix,
const Stmt *Parent, const ParenExpr *Parens);
- void issueDiag(const ASTContext &Context, SourceLocation Loc,
+ bool issueDiag(const ASTContext &Context, SourceLocation Loc,
StringRef Description, SourceRange ReplacementRange,
StringRef Replacement);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 7fcf5cfbe3783..4df4daff7cf1e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -452,7 +452,8 @@ Changes in existing checks
- Improved :doc:`readability-simplify-boolean-expr
<clang-tidy/checks/readability/simplify-boolean-expr>` check to avoid to emit
- warning for macro when IgnoreMacro option is enabled.
+ warning for macro when IgnoreMacro option is enabled and improve messages
+ when auto-fix does not work.
- Improved :doc:`readability-static-definition-in-anonymous-namespace
<clang-tidy/checks/readability/static-definition-in-anonymous-namespace>`
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp
index c14438aa93801..bad1055a01904 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp
@@ -353,6 +353,23 @@ bool conditional_return_statements(int i) {
// CHECK-FIXES: {{^}} return i == 0;{{$}}
// CHECK-FIXES-NEXT: {{^}$}}
+bool conditional_return_statements_no_fix_1(int i) {
+ if (i == 0) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: {{.*}} in conditional return statement
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: note: conditions that can be simplified
+ // comment
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: note: return statement that can be simplified
+}
+
+bool conditional_return_statements_no_fix_2(int i) {
+ if (i == 0) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: {{.*}} in conditional return statement
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: note: conditions that can be simplified
+ // comment
+ else return false;
+}
+
bool conditional_return_statements_then_expr(int i, int j) {
if (i == j) return (i == 0); else return false;
}
|
|
@llvm/pr-subscribers-clang-tidy Author: Congcong Cai (HerrCai0907) ChangesFixes: #93157 Full diff: https://github.com/llvm/llvm-project/pull/96917.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
index fd4730d9c8b9c..499c88ef5d4e4 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -9,6 +9,7 @@
#include "SimplifyBooleanExprCheck.h"
#include "clang/AST/Expr.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/DiagnosticIDs.h"
#include "clang/Lex/Lexer.h"
#include "llvm/Support/SaveAndRestore.h"
@@ -702,7 +703,8 @@ bool SimplifyBooleanExprCheck::canBeBypassed(const Stmt *S) const {
return IgnoreMacros && S->getBeginLoc().isMacroID();
}
-void SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
+/// @brief return true when replacement created.
+bool SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
SourceLocation Loc,
StringRef Description,
SourceRange ReplacementRange,
@@ -712,8 +714,10 @@ void SimplifyBooleanExprCheck::issueDiag(const ASTContext &Context,
Context.getSourceManager(), getLangOpts());
DiagnosticBuilder Diag = diag(Loc, Description);
- if (!containsDiscardedTokens(Context, CharRange))
+ const bool HasReplacement = !containsDiscardedTokens(Context, CharRange);
+ if (HasReplacement)
Diag << FixItHint::CreateReplacement(CharRange, Replacement);
+ return HasReplacement;
}
void SimplifyBooleanExprCheck::replaceWithThenStatement(
@@ -751,8 +755,18 @@ void SimplifyBooleanExprCheck::replaceWithReturnCondition(
replacementExpression(Context, Negated, If->getCond());
std::string Replacement = ("return " + Condition + Terminator).str();
SourceLocation Start = BoolLiteral->getBeginLoc();
- issueDiag(Context, Start, SimplifyConditionalReturnDiagnostic,
- If->getSourceRange(), Replacement);
+
+ const bool HasReplacement =
+ issueDiag(Context, Start, SimplifyConditionalReturnDiagnostic,
+ If->getSourceRange(), Replacement);
+
+ if (!HasReplacement) {
+ const SourceRange ConditionRange = If->getCond()->getSourceRange();
+ if (ConditionRange.isValid())
+ diag(ConditionRange.getBegin(), "conditions that can be simplified",
+ DiagnosticIDs::Note)
+ << ConditionRange;
+ }
}
void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
@@ -760,9 +774,23 @@ void SimplifyBooleanExprCheck::replaceCompoundReturnWithCondition(
const IfStmt *If, const Expr *ThenReturn) {
const std::string Replacement =
"return " + replacementExpression(Context, Negated, If->getCond());
- issueDiag(Context, ThenReturn->getBeginLoc(),
- SimplifyConditionalReturnDiagnostic,
- SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+
+ const bool HasReplacement = issueDiag(
+ Context, ThenReturn->getBeginLoc(), SimplifyConditionalReturnDiagnostic,
+ SourceRange(If->getBeginLoc(), Ret->getEndLoc()), Replacement);
+
+ if (!HasReplacement) {
+ const SourceRange ConditionRange = If->getCond()->getSourceRange();
+ if (ConditionRange.isValid())
+ diag(ConditionRange.getBegin(), "conditions that can be simplified",
+ DiagnosticIDs::Note)
+ << ConditionRange;
+ const SourceRange ReturnRange = Ret->getSourceRange();
+ if (ReturnRange.isValid())
+ diag(ReturnRange.getBegin(), "return statement that can be simplified",
+ DiagnosticIDs::Note)
+ << ReturnRange;
+ }
}
void SimplifyBooleanExprCheck::replaceWithAssignment(const ASTContext &Context,
diff --git a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
index 63c3caa01e01a..2ea6968798408 100644
--- a/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -60,7 +60,7 @@ class SimplifyBooleanExprCheck : public ClangTidyCheck {
const BinaryOperator *Inner, bool TryOfferFix,
const Stmt *Parent, const ParenExpr *Parens);
- void issueDiag(const ASTContext &Context, SourceLocation Loc,
+ bool issueDiag(const ASTContext &Context, SourceLocation Loc,
StringRef Description, SourceRange ReplacementRange,
StringRef Replacement);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 7fcf5cfbe3783..4df4daff7cf1e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -452,7 +452,8 @@ Changes in existing checks
- Improved :doc:`readability-simplify-boolean-expr
<clang-tidy/checks/readability/simplify-boolean-expr>` check to avoid to emit
- warning for macro when IgnoreMacro option is enabled.
+ warning for macro when IgnoreMacro option is enabled and improve messages
+ when auto-fix does not work.
- Improved :doc:`readability-static-definition-in-anonymous-namespace
<clang-tidy/checks/readability/static-definition-in-anonymous-namespace>`
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp
index c14438aa93801..bad1055a01904 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/simplify-boolean-expr.cpp
@@ -353,6 +353,23 @@ bool conditional_return_statements(int i) {
// CHECK-FIXES: {{^}} return i == 0;{{$}}
// CHECK-FIXES-NEXT: {{^}$}}
+bool conditional_return_statements_no_fix_1(int i) {
+ if (i == 0) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: {{.*}} in conditional return statement
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: note: conditions that can be simplified
+ // comment
+ return false;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: note: return statement that can be simplified
+}
+
+bool conditional_return_statements_no_fix_2(int i) {
+ if (i == 0) return true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: {{.*}} in conditional return statement
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: note: conditions that can be simplified
+ // comment
+ else return false;
+}
+
bool conditional_return_statements_then_expr(int i, int j) {
if (i == j) return (i == 0); else return false;
}
|
PiotrZSL
approved these changes
Jun 28, 2024
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes: #93157