[clang-tools-extra] [clang-tidy][bugprone-posix-return] support integer literals as LHS (PR #109302)
Congcong Cai via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 20 07:40:32 PDT 2024
https://github.com/HerrCai0907 updated https://github.com/llvm/llvm-project/pull/109302
>From cc2c798193722b3a537c76e74981ff767d064efa Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Thu, 19 Sep 2024 23:46:16 +0800
Subject: [PATCH 1/3] [clang-tidy][bugprone-posix-return] support integer
literals as LHS
Refactor matches to give more generic checker.
---
.../clang-tidy/bugprone/PosixReturnCheck.cpp | 61 +++++++++++--------
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../checkers/bugprone/posix-return.cpp | 25 +++++++-
3 files changed, 62 insertions(+), 28 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
index 378427a1eab000..9d70039080d332 100644
--- a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
@@ -7,19 +7,17 @@
//===----------------------------------------------------------------------===//
#include "PosixReturnCheck.h"
-#include "../utils/Matchers.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
namespace clang::tidy::bugprone {
-static StringRef getFunctionSpelling(const MatchFinder::MatchResult &Result,
- const char *BindingStr) {
- const CallExpr *MatchedCall = cast<CallExpr>(
- (Result.Nodes.getNodeAs<BinaryOperator>(BindingStr))->getLHS());
+static StringRef getFunctionSpelling(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedCall = Result.Nodes.getNodeAs<CallExpr>("call");
const SourceManager &SM = *Result.SourceManager;
return Lexer::getSourceText(CharSourceRange::getTokenRange(
MatchedCall->getCallee()->getSourceRange()),
@@ -27,32 +25,40 @@ static StringRef getFunctionSpelling(const MatchFinder::MatchResult &Result,
}
void PosixReturnCheck::registerMatchers(MatchFinder *Finder) {
+ const auto PosixCall =
+ callExpr(callee(functionDecl(
+ anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
+ unless(hasName("::posix_openpt")))))
+ .bind("call");
+ const auto ZeroIntegerLiteral = integerLiteral(equals(0));
+ const auto NegIntegerLiteral =
+ unaryOperator(hasOperatorName("-"), hasUnaryOperand(integerLiteral()));
+
Finder->addMatcher(
binaryOperator(
- hasOperatorName("<"),
- hasLHS(callExpr(callee(functionDecl(
- anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
- unless(hasName("::posix_openpt")))))),
- hasRHS(integerLiteral(equals(0))))
+ anyOf(allOf(hasOperatorName("<"), hasLHS(PosixCall),
+ hasRHS(ZeroIntegerLiteral)),
+ allOf(hasOperatorName(">"), hasLHS(ZeroIntegerLiteral),
+ hasRHS(PosixCall))))
.bind("ltzop"),
this);
Finder->addMatcher(
binaryOperator(
- hasOperatorName(">="),
- hasLHS(callExpr(callee(functionDecl(
- anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
- unless(hasName("::posix_openpt")))))),
- hasRHS(integerLiteral(equals(0))))
+ anyOf(allOf(hasOperatorName(">="), hasLHS(PosixCall),
+ hasRHS(ZeroIntegerLiteral)),
+ allOf(hasOperatorName("<="), hasLHS(ZeroIntegerLiteral),
+ hasRHS(PosixCall))))
.bind("atop"),
this);
+ Finder->addMatcher(binaryOperator(hasAnyOperatorName("==", "!="),
+ hasOperands(PosixCall, NegIntegerLiteral))
+ .bind("binop"),
+ this);
Finder->addMatcher(
- binaryOperator(
- hasAnyOperatorName("==", "!=", "<=", "<"),
- hasLHS(callExpr(callee(functionDecl(
- anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
- unless(hasName("::posix_openpt")))))),
- hasRHS(unaryOperator(hasOperatorName("-"),
- hasUnaryOperand(integerLiteral()))))
+ binaryOperator(anyOf(allOf(hasAnyOperatorName("<=", "<"),
+ hasLHS(PosixCall), hasRHS(NegIntegerLiteral)),
+ allOf(hasAnyOperatorName(">", ">="),
+ hasLHS(NegIntegerLiteral), hasRHS(PosixCall))))
.bind("binop"),
this);
}
@@ -61,10 +67,13 @@ void PosixReturnCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *LessThanZeroOp =
Result.Nodes.getNodeAs<BinaryOperator>("ltzop")) {
SourceLocation OperatorLoc = LessThanZeroOp->getOperatorLoc();
+ char NewBinOp = LessThanZeroOp->getOpcode() == BinaryOperator::Opcode::BO_LT
+ ? '>'
+ : '<';
diag(OperatorLoc, "the comparison always evaluates to false because %0 "
"always returns non-negative values")
- << getFunctionSpelling(Result, "ltzop")
- << FixItHint::CreateReplacement(OperatorLoc, Twine(">").str());
+ << getFunctionSpelling(Result)
+ << FixItHint::CreateReplacement(OperatorLoc, Twine(NewBinOp).str());
return;
}
if (const auto *AlwaysTrueOp =
@@ -72,12 +81,12 @@ void PosixReturnCheck::check(const MatchFinder::MatchResult &Result) {
diag(AlwaysTrueOp->getOperatorLoc(),
"the comparison always evaluates to true because %0 always returns "
"non-negative values")
- << getFunctionSpelling(Result, "atop");
+ << getFunctionSpelling(Result);
return;
}
const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
diag(BinOp->getOperatorLoc(), "%0 only returns non-negative values")
- << getFunctionSpelling(Result, "binop");
+ << getFunctionSpelling(Result);
}
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d032cef6b76164..3729a479abeff2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -126,6 +126,10 @@ Changes in existing checks
usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or
subtracting from a pointer.
+- Improved :doc:`bugprone-posix-return
+ <clang-tidy/checks/bugprone/posix-return>` check to support integer literals
+ as LHS and posix call as RHS of comparison.
+
- Improved :doc:`cert-flp30-c <clang-tidy/checks/cert/flp30-c>` check to
fix false positive that floating point variable is only used in increment
expression.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/posix-return.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/posix-return.cpp
index 271893c7070695..76d447a71d68b3 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/posix-return.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/posix-return.cpp
@@ -74,6 +74,9 @@ void warningLessThanZero() {
if (pthread_yield() < 0) {}
// CHECK-MESSAGES: :[[@LINE-1]]:23: warning:
// CHECK-FIXES: pthread_yield() > 0
+ if (0 > pthread_yield() ) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning:
+ // CHECK-FIXES: 0 < pthread_yield()
}
@@ -90,7 +93,8 @@ void warningAlwaysTrue() {
// CHECK-MESSAGES: :[[@LINE-1]]:31: warning:
if (pthread_yield() >= 0) {}
// CHECK-MESSAGES: :[[@LINE-1]]:23: warning:
-
+ if (0 <= pthread_yield()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning:
}
void warningEqualsNegative() {
@@ -120,7 +124,14 @@ void warningEqualsNegative() {
// CHECK-MESSAGES: :[[@LINE-1]]:46: warning:
if (pthread_create(NULL, NULL, NULL, NULL) < -1) {}
// CHECK-MESSAGES: :[[@LINE-1]]:46: warning:
-
+ if (-1 == pthread_create(NULL, NULL, NULL, NULL)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning:
+ if (-1 != pthread_create(NULL, NULL, NULL, NULL)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning:
+ if (-1 >= pthread_create(NULL, NULL, NULL, NULL)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning:
+ if (-1 > pthread_create(NULL, NULL, NULL, NULL)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning:
}
void WarningWithMacro() {
@@ -162,6 +173,16 @@ void noWarning() {
if (posix_openpt(0) < -1) {}
if (posix_fadvise(0, 0, 0, 0) <= 0) {}
if (posix_fadvise(0, 0, 0, 0) == 1) {}
+ if (0 > posix_openpt(0)) {}
+ if (0 >= posix_openpt(0)) {}
+ if (-1 == posix_openpt(0)) {}
+ if (-1 != posix_openpt(0)) {}
+ if (-1 >= posix_openpt(0)) {}
+ if (-1 > posix_openpt(0)) {}
+ if (posix_fadvise(0, 0, 0, 0) <= 0) {}
+ if (posix_fadvise(0, 0, 0, 0) == 1) {}
+ if (0 >= posix_fadvise(0, 0, 0, 0)) {}
+ if (1 == posix_fadvise(0, 0, 0, 0)) {}
}
namespace i {
>From 4c6b496dd39027cd67aa4119f64c7c655c6e3eb3 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Fri, 20 Sep 2024 22:39:07 +0800
Subject: [PATCH 2/3] Update
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
Co-authored-by: EugeneZelenko <eugene.zelenko at gmail.com>
---
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
index 9d70039080d332..e4ceee127a3978 100644
--- a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
@@ -67,7 +67,7 @@ void PosixReturnCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *LessThanZeroOp =
Result.Nodes.getNodeAs<BinaryOperator>("ltzop")) {
SourceLocation OperatorLoc = LessThanZeroOp->getOperatorLoc();
- char NewBinOp = LessThanZeroOp->getOpcode() == BinaryOperator::Opcode::BO_LT
+ const char NewBinOp = LessThanZeroOp->getOpcode() == BinaryOperator::Opcode::BO_LT
? '>'
: '<';
diag(OperatorLoc, "the comparison always evaluates to false because %0 "
>From b42278e592a9d64ba6868ee6eb5663cb8306e16b Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Fri, 20 Sep 2024 22:40:23 +0800
Subject: [PATCH 3/3] Update ReleaseNotes.rst
---
clang-tools-extra/docs/ReleaseNotes.rst | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3729a479abeff2..277040a63b2623 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -121,15 +121,15 @@ Changes in existing checks
<clang-tidy/checks/bugprone/forwarding-reference-overload>` check by fixing
a crash when determining if an ``enable_if[_t]`` was found.
+- Improved :doc:`bugprone-posix-return
+ <clang-tidy/checks/bugprone/posix-return>` check to support integer literals
+ as LHS and posix call as RHS of comparison.
+
- Improved :doc:`bugprone-sizeof-expression
<clang-tidy/checks/bugprone/sizeof-expression>` check to find suspicious
usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or
subtracting from a pointer.
-- Improved :doc:`bugprone-posix-return
- <clang-tidy/checks/bugprone/posix-return>` check to support integer literals
- as LHS and posix call as RHS of comparison.
-
- Improved :doc:`cert-flp30-c <clang-tidy/checks/cert/flp30-c>` check to
fix false positive that floating point variable is only used in increment
expression.
More information about the cfe-commits
mailing list