[clang-tools-extra] FIX : bugprone-too-small-loop-variable - false-negative when const variable is used as loop bound (PR #81183)

[Shourya Goel via cfe-commits]

https://github.com/Sh0g0-1758 created https://github.com/llvm/llvm-project/pull/81183

Fixes: #79580

**PR SUMMARY**: Changed LibASTMatcher to give an appropriate warning when a const loop bound is initialized with a function declaration. 

>From 88dac6713284ee4f0b7ce73c944f78085412645f Mon Sep 17 00:00:00 2001
From: Sh0g0-1758 <shouryagoel10000 at gmail.com>
Date: Fri, 9 Feb 2024 01:21:14 +0530
Subject: [PATCH 1/2] Fix : bugprone-too-small-loop-variable

---
 .../bugprone/TooSmallLoopVariableCheck.cpp           | 12 ++++++------
 clang-tools-extra/docs/ReleaseNotes.rst              |  3 +++
 .../checks/bugprone/too-small-loop-variable.rst      |  2 ++
 .../checkers/bugprone/too-small-loop-variable.cpp    | 12 ++++++++++++
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
index 8ba8b893e03a6f..8f6547f4ea9e65 100644
--- a/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
@@ -81,12 +81,12 @@ void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
 
   // We are interested in only those cases when the loop bound is a variable
   // value (not const, enum, etc.).
-  StatementMatcher LoopBoundMatcher =
-      expr(ignoringParenImpCasts(allOf(hasType(isInteger()),
-                                       unless(integerLiteral()),
-                                       unless(hasType(isConstQualified())),
-                                       unless(hasType(enumType())))))
-          .bind(LoopUpperBoundName);
+ StatementMatcher LoopBoundMatcher =
+       expr(ignoringParenImpCasts(allOf(hasType(isInteger()),
+                                        unless(integerLiteral()),
+                                        unless(allOf(hasType(isConstQualified()), declRefExpr(to(varDecl(anyOf(hasInitializer(ignoringParenImpCasts(integerLiteral())), isConstexpr(), isConstinit())))))),
+                                        unless(hasType(enumType())))))
+           .bind(LoopUpperBoundName);
 
   // We use the loop increment expression only to make sure we found the right
   // loop variable.
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index e50914aed5f07a..17b1349d20db8d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -169,6 +169,9 @@ Miscellaneous
   option is specified. Now ``clang-apply-replacements`` applies formatting only with
   the option.
 
+- Fixed incorrect implementation of ``too-small-loop-variable`` check when a const loop
+  variable is initialized with a function declaration.
+
 Improvements to include-fixer
 -----------------------------
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/too-small-loop-variable.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/too-small-loop-variable.rst
index 0f45cc2fe11463..9ee23be45cfea0 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/too-small-loop-variable.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/too-small-loop-variable.rst
@@ -44,3 +44,5 @@ a larger user input.
     for (unsigned i = 0; i < size; ++i) {} // no warning with MagnitudeBitsUpperLimit = 31 on a system where unsigned is 32-bit
     for (int i = 0; i < size; ++i) {} // warning with MagnitudeBitsUpperLimit = 31 on a system where int is 32-bit
   }
+
+``-Wtautological-constant-out-of-range-compare`` compiler warning should also be used.
\ No newline at end of file
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp
index 3229deb93bada8..113150b168650b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp
@@ -93,6 +93,18 @@ void voidBadForLoopWithMacroBound() {
   }
 }
 
+unsigned int getVal() {
+    return 300;
+}
+
+// The iteration's upper bound has a function declaration.
+void voidBadForLoop8() {
+  const unsigned int l = getVal();
+  for (unsigned char i = 0; i < l; ++i) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: loop variable has narrower type 'unsigned char' than iteration's upper bound 'const unsigned int' [bugprone-too-small-loop-variable]
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 /// Correct loops: we should not warn here.
 

>From 41acf22b481ba6800c1d07e96d1d3ba8052b20a7 Mon Sep 17 00:00:00 2001
From: Sh0g0-1758 <shouryagoel10000 at gmail.com>
Date: Fri, 9 Feb 2024 01:21:39 +0530
Subject: [PATCH 2/2] Ran git clang Formatter

---
 .../bugprone/TooSmallLoopVariableCheck.cpp       | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
index 8f6547f4ea9e65..a73d46f01d9b2d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
@@ -81,12 +81,16 @@ void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
 
   // We are interested in only those cases when the loop bound is a variable
   // value (not const, enum, etc.).
- StatementMatcher LoopBoundMatcher =
-       expr(ignoringParenImpCasts(allOf(hasType(isInteger()),
-                                        unless(integerLiteral()),
-                                        unless(allOf(hasType(isConstQualified()), declRefExpr(to(varDecl(anyOf(hasInitializer(ignoringParenImpCasts(integerLiteral())), isConstexpr(), isConstinit())))))),
-                                        unless(hasType(enumType())))))
-           .bind(LoopUpperBoundName);
+  StatementMatcher LoopBoundMatcher =
+      expr(ignoringParenImpCasts(allOf(
+               hasType(isInteger()), unless(integerLiteral()),
+               unless(allOf(
+                   hasType(isConstQualified()),
+                   declRefExpr(to(varDecl(anyOf(
+                       hasInitializer(ignoringParenImpCasts(integerLiteral())),
+                       isConstexpr(), isConstinit())))))),
+               unless(hasType(enumType())))))
+          .bind(LoopUpperBoundName);
 
   // We use the loop increment expression only to make sure we found the right
   // loop variable.