[clang-tidy] Simplify too-small loop variable check

[steveire]

Summary

Don't issue a warning from within a template declaration which is most
likely not actionable.

[njames93]

I'm not sure about this. The warning is good and addresses a real problem.
I'm not sure what the infrastructure surrounding this is but in clang it would show a "stack trace" of instantiations that result in this warning. Maybe we should think of something similar in clang tidy.

[steveire]

Update

[steveire]

In D96142#2545078, @njames93 wrote:

I'm not sure about this. The warning is good and addresses a real problem.

Well, I've made the diagnostic better anyway.

[njames93]

In D96142#2547418, @steveire wrote:

In D96142#2545078, @njames93 wrote:

I'm not sure about this. The warning is good and addresses a real problem.

Well, I've made the diagnostic better anyway.

I'm not sure its an improvement. If the template is never instantiated with something that would trigger this, we shouldn't really warn on it.
There are plenty of times when certain template definitions will error if certain template parameters are passed to them.
This is well defined and no compiler will ever warn on those situations.

[steveire]

In D96142#2564265, @njames93 wrote:

In D96142#2547418, @steveire wrote:

In D96142#2545078, @njames93 wrote:

I'm not sure about this. The warning is good and addresses a real problem.

Well, I've made the diagnostic better anyway.

I'm not sure its an improvement. If the template is never instantiated with something that would trigger this, we shouldn't really warn on it.
There are plenty of times when certain template definitions will error if certain template parameters are passed to them.
This is well defined and no compiler will ever warn on those situations.

Yes, fair point. I still don't think the existing diagnostic is valuable, but removing the diagnostic would be the inevitable result of a change like this.

clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.h

 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-too-small-loop-variable.html
 class TooSmallLoopVariableCheck : public ClangTidyCheck {
 public:
   TooSmallLoopVariableCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   const unsigned MagnitudeBitsUpperLimit;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TOOSMALLLOOPVARIABLECHECK_H

clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp

 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 static constexpr llvm::StringLiteral LoopName =
     llvm::StringLiteral("forLoopName");
 static constexpr llvm::StringLiteral LoopVarName =
     llvm::StringLiteral("loopVar");
-static constexpr llvm::StringLiteral LoopVarCastName =
-    llvm::StringLiteral("loopVarCast");
 static constexpr llvm::StringLiteral LoopUpperBoundName =
     llvm::StringLiteral("loopUpperBound");
 static constexpr llvm::StringLiteral LoopIncrementName =
     llvm::StringLiteral("loopIncrement");
 
 TooSmallLoopVariableCheck::TooSmallLoopVariableCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       MagnitudeBitsUpperLimit(Options.get("MagnitudeBitsUpperLimit", 16U)) {}
 
 void TooSmallLoopVariableCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "MagnitudeBitsUpperLimit", MagnitudeBitsUpperLimit);
 }
 
 /// The matcher for loops with suspicious integer loop variable.
 ///
 /// In this general example, assuming 'j' and 'k' are of integral type:
 /// \code
 ///   for (...; j < 3 + 2; ++k) { ... }
 /// \endcode
 /// The following string identifiers are bound to these parts of the AST:
 ///   LoopVarName: 'j' (as a VarDecl)
-///   LoopVarCastName: 'j' (after implicit conversion)
 ///   LoopUpperBoundName: '3 + 2' (as an Expr)
 ///   LoopIncrementName: 'k' (as an Expr)
 ///   LoopName: The entire for loop (as a ForStmt)
 ///
 void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
   StatementMatcher LoopVarMatcher =
-      expr(
-          ignoringParenImpCasts(declRefExpr(to(varDecl(hasType(isInteger()))))))
+      declRefExpr(anyOf(hasType(isInteger()), isTypeDependent()))
           .bind(LoopVarName);
 
-  // We need to catch only those comparisons which contain any integer cast.
-  StatementMatcher LoopVarConversionMatcher = traverse(
-      TK_AsIs, implicitCastExpr(hasImplicitDestinationType(isInteger()),
-                                has(ignoringParenImpCasts(LoopVarMatcher)))
-                   .bind(LoopVarCastName));
-
-  // 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())))))
+  auto LoopBoundMatcher =
+      expr(anyOf(allOf(hasType(isInteger()), unless(integerLiteral()),
+                       unless(hasType(enumType()))),
+                 isTypeDependent()))
           .bind(LoopUpperBoundName);
 
   // We use the loop increment expression only to make sure we found the right
   // loop variable.
-  StatementMatcher IncrementMatcher =
-      expr(ignoringParenImpCasts(hasType(isInteger()))).bind(LoopIncrementName);
+  auto IncrementMatcher = expr(anyOf(hasType(isInteger()), isTypeDependent()))
+                              .bind(LoopIncrementName);
 
   Finder->addMatcher(
       forStmt(
           hasCondition(anyOf(
-              binaryOperator(hasOperatorName("<"),
-                             hasLHS(LoopVarConversionMatcher),
+              binaryOperator(hasOperatorName("<"), hasLHS(LoopVarMatcher),
                              hasRHS(LoopBoundMatcher)),
-              binaryOperator(hasOperatorName("<="),
-                             hasLHS(LoopVarConversionMatcher),
+              binaryOperator(hasOperatorName("<="), hasLHS(LoopVarMatcher),
                              hasRHS(LoopBoundMatcher)),
               binaryOperator(hasOperatorName(">"), hasLHS(LoopBoundMatcher),
-                             hasRHS(LoopVarConversionMatcher)),
+                             hasRHS(LoopVarMatcher)),
               binaryOperator(hasOperatorName(">="), hasLHS(LoopBoundMatcher),
-                             hasRHS(LoopVarConversionMatcher)))),
+                             hasRHS(LoopVarMatcher)))),
           hasIncrement(IncrementMatcher))
           .bind(LoopName),
       this);
 }
 
 /// Returns the magnitude bits of an integer type.
 static unsigned calcMagnitudeBits(const ASTContext &Context,
                                   const QualType &IntExprType) {

 
 void TooSmallLoopVariableCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *LoopVar = Result.Nodes.getNodeAs<Expr>(LoopVarName);
   const auto *UpperBound =
       Result.Nodes.getNodeAs<Expr>(LoopUpperBoundName)->IgnoreParenImpCasts();
   const auto *LoopIncrement =
       Result.Nodes.getNodeAs<Expr>(LoopIncrementName)->IgnoreParenImpCasts();
 
+  if (LoopVar->isTypeDependent()) {
+    diag(LoopVar->getBeginLoc(),
+         "loop variable is template dependent and might be instantiated with a "
+         "type which is too small.");
+    return;
+  }
+  if (UpperBound->isTypeDependent()) {
+    diag(UpperBound->getBeginLoc(),
+         "boundary variable is template dependent and might be instantiated "
+         "with a type which is too large for the loop variable.");
+    return;
+  }
+
   // We matched the loop variable incorrectly.
   if (LoopVar->getType() != LoopIncrement->getType())
     return;
 
   QualType LoopVarType = LoopVar->getType();
   QualType UpperBoundType = UpperBound->getType();
 
   ASTContext &Context = *Result.Context;

clang-tools-extra/test/clang-tidy/checkers/bugprone-too-small-loop-variable.cpp

for (short i = 0; i < size; ++i) {
     // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
   }
 }
 
 // The iteration's upper bound has a template dependent type.
 template <class T>
 void doSomething() {
   for (T i = 0; i < size(); ++i) {
-    // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable has narrower type 'short' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+    // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: loop variable is template dependent and might be instantiated with a type which is too small.
+  }
+}
+
+template <class T>
+struct Templ {
+};
+
+template <class T>
+void dependentBoundary() {
+  Templ<T> t;
+  for (int i = 0; i < t.size(); ++i) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: boundary variable is template dependent and might be instantiated with a type which is too large for the loop variable.
   }
 }
 
 void voidForLoopInstantiation() {
   // This line does not trigger the warning.
   doSomething<long>();
   // This one triggers the warning.
   doSomething<short>();