[clang-tools-extra] [clang-tidy] Keep parentheses when replacing index access in `sizeof` calls (PR #82166)

[Danny Mösch via cfe-commits]

https://github.com/SimplyDanny updated https://github.com/llvm/llvm-project/pull/82166

>From cd3c0d0d4b3133927a830c249a541510bacfabf8 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/3] [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 dff0ca1e6f12e2d11838359cc1e4b7833c02cab2 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/3] 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 9fc694b7a6015ae749b9219c935cbf7d6833a6e3 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/3] 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) {