[clang-tools-extra] [clang-tidy] Keep parentheses when replacing index access in `sizeof` calls (PR #82166)
Danny Mösch via cfe-commits
cfe-commits at lists.llvm.org
Sun Feb 18 13:36:12 PST 2024
https://github.com/SimplyDanny updated https://github.com/llvm/llvm-project/pull/82166
>From 7aa267d752408fedcf14b62cd015d90de6719459 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moesch at icloud.com>
Date: Sun, 18 Feb 2024 14:46:54 +0100
Subject: [PATCH 1/5] [clang-tidy] Keep parentheses when replacing index access
in `sizeof` calls
---
.../clang-tidy/modernize/LoopConvertCheck.cpp | 8 ++++++--
clang-tools-extra/docs/ReleaseNotes.rst | 5 +++++
.../checkers/modernize/loop-convert-basic.cpp | 13 ++++++++++++-
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index f0791da143ad9d..6c9a330d733407 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -711,8 +711,12 @@ void LoopConvertCheck::doConversion(
if (const auto *Paren = Parents[0].get<ParenExpr>()) {
// Usage.Expression will be replaced with the new index variable,
// and parenthesis around a simple DeclRefExpr can always be
- // removed.
- Range = Paren->getSourceRange();
+ // removed except in case of a `sizeof` operator call.
+ auto GrandParents = Context->getParents(*Paren);
+ if (GrandParents.size() != 1 ||
+ !GrandParents[0].get<UnaryExprOrTypeTraitExpr>()) {
+ Range = Paren->getSourceRange();
+ }
} else if (const auto *UOP = Parents[0].get<UnaryOperator>()) {
// If we are taking the address of the loop variable, then we must
// not use a copy, as it would mean taking the address of the loop's
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 7ca7037e2a6a4f..58629426216ba8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -170,6 +170,11 @@ Changes in existing checks
`AllowStringArrays` option, enabling the exclusion of array types with deduced
length initialized from string literals.
+- Improved :doc:`modernize-loop-convert
+ <clang-tidy/checks/modernize/loop-convert>` check by ensuring that fix-its
+ don't remove parentheses used in ``sizeof`` calls when they have array index
+ accesses as arguments.
+
- Improved :doc:`modernize-use-override
<clang-tidy/checks/modernize/use-override>` check to also remove any trailing
whitespace when deleting the ``virtual`` keyword.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
index c29fbc9f9b23b7..02601b6320491a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-loop-convert %t -- -- -I %S/Inputs/loop-convert
+// RUN: %check_clang_tidy %s modernize-loop-convert %t -- -- -I %S/Inputs/loop-convert -isystem %clang_tidy_headers
+
+#include <cstddef>
#include "structures.h"
@@ -88,6 +90,15 @@ void f() {
// CHECK-FIXES-NEXT: printf("Fibonacci number %d has address %p\n", I, &I);
// CHECK-FIXES-NEXT: Sum += I + 2;
+ int Matrix[N][12];
+ size_t size = 0;
+ for (int I = 0; I < N; ++I) {
+ size += sizeof(Matrix[I]) + sizeof Matrix[I];
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (auto & I : Matrix)
+ // CHECK-FIXES-NEXT: size += sizeof(I) + sizeof I;
+
Val Teas[N];
for (int I = 0; I < N; ++I) {
Teas[I].g();
>From 47d265569f26f7fab74ebd75be752807e2d04e75 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moesch at icloud.com>
Date: Sun, 18 Feb 2024 17:33:24 +0100
Subject: [PATCH 2/5] Avoid auto
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 6c9a330d733407..cac742cd46331e 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -706,13 +706,13 @@ void LoopConvertCheck::doConversion(
ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow
? VarNameOrStructuredBinding + "."
: VarNameOrStructuredBinding;
- auto Parents = Context->getParents(*Usage.Expression);
+ const DynTypedNodeList Parents = Context->getParents(*Usage.Expression);
if (Parents.size() == 1) {
if (const auto *Paren = Parents[0].get<ParenExpr>()) {
// Usage.Expression will be replaced with the new index variable,
// and parenthesis around a simple DeclRefExpr can always be
// removed except in case of a `sizeof` operator call.
- auto GrandParents = Context->getParents(*Paren);
+ const DynTypedNodeList GrandParents = Context->getParents(*Paren);
if (GrandParents.size() != 1 ||
!GrandParents[0].get<UnaryExprOrTypeTraitExpr>()) {
Range = Paren->getSourceRange();
>From 60ab6db8c5c5d981fafd27e5ed4a54ce6e5a3a0d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moesch at icloud.com>
Date: Sun, 18 Feb 2024 17:35:54 +0100
Subject: [PATCH 3/5] Add more test cases
---
.../checkers/modernize/loop-convert-basic.cpp | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
index 02601b6320491a..5eb30020312d44 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
@@ -93,11 +93,15 @@ void f() {
int Matrix[N][12];
size_t size = 0;
for (int I = 0; I < N; ++I) {
- size += sizeof(Matrix[I]) + sizeof Matrix[I];
+ size += sizeof(Matrix[I]);
+ size += sizeof Matrix[I];
+ size += sizeof((Matrix[I]));
}
- // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
// CHECK-FIXES: for (auto & I : Matrix)
- // CHECK-FIXES-NEXT: size += sizeof(I) + sizeof I;
+ // CHECK-FIXES-NEXT: size += sizeof(I);
+ // CHECK-FIXES-NEXT: size += sizeof I;
+ // CHECK-FIXES-NEXT: size += sizeof(I);
Val Teas[N];
for (int I = 0; I < N; ++I) {
>From 47b6fc1b85973bb1a51dd656d6c8a7a9a1aee1a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moesch at icloud.com>
Date: Sun, 18 Feb 2024 22:34:06 +0100
Subject: [PATCH 4/5] Compare with `nullptr`
---
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index cac742cd46331e..3229e302eb4322 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -714,7 +714,7 @@ void LoopConvertCheck::doConversion(
// removed except in case of a `sizeof` operator call.
const DynTypedNodeList GrandParents = Context->getParents(*Paren);
if (GrandParents.size() != 1 ||
- !GrandParents[0].get<UnaryExprOrTypeTraitExpr>()) {
+ GrandParents[0].get<UnaryExprOrTypeTraitExpr>() == nullptr) {
Range = Paren->getSourceRange();
}
} else if (const auto *UOP = Parents[0].get<UnaryOperator>()) {
>From afb68c4acce43f3b5651a0d02320b3b969c11977 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Danny=20M=C3=B6sch?= <danny.moesch at icloud.com>
Date: Sun, 18 Feb 2024 22:34:56 +0100
Subject: [PATCH 5/5] Use built-in type to avoid include
---
.../clang-tidy/checkers/modernize/loop-convert-basic.cpp | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
index 5eb30020312d44..695925a5d877c9 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
@@ -1,6 +1,4 @@
-// RUN: %check_clang_tidy %s modernize-loop-convert %t -- -- -I %S/Inputs/loop-convert -isystem %clang_tidy_headers
-
-#include <cstddef>
+// RUN: %check_clang_tidy %s modernize-loop-convert %t -- -- -I %S/Inputs/loop-convert
#include "structures.h"
@@ -91,7 +89,7 @@ void f() {
// CHECK-FIXES-NEXT: Sum += I + 2;
int Matrix[N][12];
- size_t size = 0;
+ unsigned size = 0;
for (int I = 0; I < N; ++I) {
size += sizeof(Matrix[I]);
size += sizeof Matrix[I];
More information about the cfe-commits
mailing list