[clang-tidy][bugprone-posix-return] support integer literals as LHS (#109302)

Refactor matches to give more generic checker.

---------

Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>

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,
+static StringRef getFunctionSpelling(const MatchFinder::MatchResult &Result) {
-                                     const char *BindingStr) {
+  const auto *MatchedCall = Result.Nodes.getNodeAs<CallExpr>("call");
-  const CallExpr *MatchedCall = cast<CallExpr>(
-      (Result.Nodes.getNodeAs<BinaryOperator>(BindingStr))->getLHS());
   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("<"),
+          anyOf(allOf(hasOperatorName("<"), hasLHS(PosixCall),
-          hasLHS(callExpr(callee(functionDecl(
+                      hasRHS(ZeroIntegerLiteral)),
-              anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
+                allOf(hasOperatorName(">"), hasLHS(ZeroIntegerLiteral),
-              unless(hasName("::posix_openpt")))))),
+                      hasRHS(PosixCall))))
-          hasRHS(integerLiteral(equals(0))))
           .bind("ltzop"),
       this);
   Finder->addMatcher(
       binaryOperator(
-          hasOperatorName(">="),
+          anyOf(allOf(hasOperatorName(">="), hasLHS(PosixCall),
-          hasLHS(callExpr(callee(functionDecl(
+                      hasRHS(ZeroIntegerLiteral)),
-              anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
+                allOf(hasOperatorName("<="), hasLHS(ZeroIntegerLiteral),
-              unless(hasName("::posix_openpt")))))),
+                      hasRHS(PosixCall))))
-          hasRHS(integerLiteral(equals(0))))
           .bind("atop"),
       this);
+  Finder->addMatcher(binaryOperator(hasAnyOperatorName("==", "!="),
+                                    hasOperands(PosixCall, NegIntegerLiteral))
+                         .bind("binop"),
+                     this);
   Finder->addMatcher(
-      binaryOperator(
+      binaryOperator(anyOf(allOf(hasAnyOperatorName("<=", "<"),
-          hasAnyOperatorName("==", "!=", "<=", "<"),
+                                 hasLHS(PosixCall), hasRHS(NegIntegerLiteral)),
-          hasLHS(callExpr(callee(functionDecl(
+                           allOf(hasAnyOperatorName(">", ">="),
-              anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
+                                 hasLHS(NegIntegerLiteral), hasRHS(PosixCall))))
-              unless(hasName("::posix_openpt")))))),
-          hasRHS(unaryOperator(hasOperatorName("-"),
-                               hasUnaryOperand(integerLiteral()))))
           .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();
+    StringRef 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")
+        << getFunctionSpelling(Result)
-        << FixItHint::CreateReplacement(OperatorLoc, Twine(">").str());
+        << FixItHint::CreateReplacement(OperatorLoc, NewBinOp);
     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

clang-tools-extra/docs/ReleaseNotes.rst

@@ -125,6 +125,10 @@ 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

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 {