[clang-tools-extra] [clang-tidy] fix false positive that floating point variable only used in increment expr in cert-flp30-c (PR #108706)
Congcong Cai via cfe-commits
cfe-commits at lists.llvm.org
Sun Sep 15 18:05:21 PDT 2024
https://github.com/HerrCai0907 updated https://github.com/llvm/llvm-project/pull/108706
>From 9fbbc1821c41a75a3a606f4a8bb0a7c43fe9dee3 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Sat, 14 Sep 2024 23:25:36 +0800
Subject: [PATCH 1/2] [clang-tidy] fix false positive that floating point
variable only used in increment expr in cert-flp30-c
Fixes: #108049
cert-flp30-c only provides non-compliant example with normal loop.
Essentially it wants to avoid that floating point variables are used as loop counters which are checked in condition expr and modified in increment expr.
This patch wants to give more precise matcheres to identify this cases.
---
.../clang-tidy/cert/FloatLoopCounter.cpp | 21 +++++++++++++----
clang-tools-extra/docs/ReleaseNotes.rst | 3 +++
.../test/clang-tidy/checkers/cert/flp30-c.c | 23 +++++++++++++------
3 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp b/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp
index a9363837597867..ecaa078cc44fa5 100644
--- a/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp
+++ b/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp
@@ -9,22 +9,33 @@
#include "FloatLoopCounter.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
using namespace clang::ast_matchers;
namespace clang::tidy::cert {
void FloatLoopCounter::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(
- forStmt(hasIncrement(expr(hasType(realFloatingPointType())))).bind("for"),
- this);
+ Finder->addMatcher(forStmt(hasIncrement(forEachDescendant(
+ declRefExpr(hasType(realFloatingPointType()),
+ to(varDecl().bind("var"))))),
+ hasCondition(forEachDescendant(declRefExpr(
+ hasType(realFloatingPointType()),
+ to(varDecl(equalsBoundNode("var")))))))
+ .bind("for"),
+ this);
}
void FloatLoopCounter::check(const MatchFinder::MatchResult &Result) {
const auto *FS = Result.Nodes.getNodeAs<ForStmt>("for");
- diag(FS->getInc()->getExprLoc(), "loop induction expression should not have "
- "floating-point type");
+ diag(FS->getInc()->getBeginLoc(), "loop induction expression should not have "
+ "floating-point type");
+
+ if (!FS->getInc()->getType()->isRealFloatingType())
+ if (const auto *V = Result.Nodes.getNodeAs<VarDecl>("var"))
+ diag(V->getBeginLoc(), "floating-point type loop induction variable",
+ DiagnosticIDs::Note);
}
} // namespace clang::tidy::cert
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 88b92830fda6b4..12638ec66c913a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,9 @@ Changes in existing checks
<clang-tidy/checks/bugprone/casting-through-void>` check to suggest replacing
the offending code with ``reinterpret_cast``, to more clearly express intent.
+- Improved :doc:`cert-flp30-c<clang-tidy/checks/cert/flp30-c>` check to
+ fix false positive that floating point variable only used in increment expr.
+
- Improved :doc:`modernize-use-std-format
<clang-tidy/checks/modernize/use-std-format>` check to support replacing
member function calls too.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cert/flp30-c.c b/clang-tools-extra/test/clang-tidy/checkers/cert/flp30-c.c
index eee16bee3d82f4..b9985887a81c53 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cert/flp30-c.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/cert/flp30-c.c
@@ -1,19 +1,28 @@
// RUN: %check_clang_tidy %s cert-flp30-c %t
float g(void);
+int c(float);
+float f = 1.0f;
+
+void match(void) {
-void func(void) {
for (float x = 0.1f; x <= 1.0f; x += 0.1f) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: loop induction expression should not have floating-point type [cert-flp30-c]
+ // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: loop induction expression should not have floating-point type [cert-flp30-c]
- float f = 1.0f;
for (; f > 0; --f) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop induction expression
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop induction expression should not have floating-point type [cert-flp30-c]
- for (;;g()) {}
- // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop induction expression
+ for (float x = 0.0f; c(x); x = g()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: loop induction expression should not have floating-point type [cert-flp30-c]
- for (int i = 0; i < 10; i += 1.0f) {}
+ for (int i=0; i < 10 && f < 2.0f; f++, i++) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: loop induction expression should not have floating-point type [cert-flp30-c]
+ // CHECK-MESSAGES: :5:1: note: floating-point type loop induction variable
+}
+void not_match(void) {
+ for (int i = 0; i < 10; i += 1.0f) {}
for (int i = 0; i < 10; ++i) {}
+ for (int i = 0; i < 10; ++i, f++) {}
+ for (int i = 0; f < 10.f; ++i) {}
}
>From 9cdaf0426efd8fe50792975aa9b497a9c50b8c1c Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Mon, 16 Sep 2024 08:59:04 +0800
Subject: [PATCH 2/2] fix
---
.../clang-tidy/cert/FloatLoopCounter.cpp | 23 +++++++++++--------
clang-tools-extra/docs/ReleaseNotes.rst | 3 ++-
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp b/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp
index ecaa078cc44fa5..46acc9f4716c5d 100644
--- a/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp
+++ b/clang-tools-extra/clang-tidy/cert/FloatLoopCounter.cpp
@@ -16,21 +16,26 @@ using namespace clang::ast_matchers;
namespace clang::tidy::cert {
void FloatLoopCounter::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(forStmt(hasIncrement(forEachDescendant(
- declRefExpr(hasType(realFloatingPointType()),
- to(varDecl().bind("var"))))),
- hasCondition(forEachDescendant(declRefExpr(
- hasType(realFloatingPointType()),
- to(varDecl(equalsBoundNode("var")))))))
- .bind("for"),
- this);
+ Finder->addMatcher(
+ forStmt(hasIncrement(forEachDescendant(
+ declRefExpr(hasType(realFloatingPointType()),
+ to(varDecl().bind("var")))
+ .bind("inc"))),
+ hasCondition(forEachDescendant(
+ declRefExpr(hasType(realFloatingPointType()),
+ to(varDecl(equalsBoundNode("var"))))
+ .bind("cond"))))
+ .bind("for"),
+ this);
}
void FloatLoopCounter::check(const MatchFinder::MatchResult &Result) {
const auto *FS = Result.Nodes.getNodeAs<ForStmt>("for");
diag(FS->getInc()->getBeginLoc(), "loop induction expression should not have "
- "floating-point type");
+ "floating-point type")
+ << Result.Nodes.getNodeAs<DeclRefExpr>("inc")->getSourceRange()
+ << Result.Nodes.getNodeAs<DeclRefExpr>("cond")->getSourceRange();
if (!FS->getInc()->getType()->isRealFloatingType())
if (const auto *V = Result.Nodes.getNodeAs<VarDecl>("var"))
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 12638ec66c913a..54e8081011ef3f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,7 +112,8 @@ Changes in existing checks
the offending code with ``reinterpret_cast``, to more clearly express intent.
- Improved :doc:`cert-flp30-c<clang-tidy/checks/cert/flp30-c>` check to
- fix false positive that floating point variable only used in increment expr.
+ fix false positive that floating point variable is only used in increment
+ expression.
- Improved :doc:`modernize-use-std-format
<clang-tidy/checks/modernize/use-std-format>` check to support replacing
More information about the cfe-commits
mailing list