[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

 ///   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()))))))
-          .bind(LoopVarName);
+      declRefExpr(to(varDecl(hasType(isInteger())))).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(allOf(hasType(isInteger()), unless(integerLiteral()),
+                 unless(hasType(enumType()))))
           .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(hasType(isInteger())).bind(LoopIncrementName);
 
   Finder->addMatcher(
       forStmt(
           hasCondition(anyOf(
               binaryOperator(hasOperatorName("<"),
                              hasLHS(LoopVarConversionMatcher),
                              hasRHS(LoopBoundMatcher)),
               binaryOperator(hasOperatorName("<="),

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]
   }
 }
 
 void voidForLoopInstantiation() {
   // This line does not trigger the warning.
   doSomething<long>();
   // This one triggers the warning.
   doSomething<short>();