[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
----------------
ztamas wrote:
> JonasToth wrote:
> > Please move these variable in the matcher function and make them `StringRef` instead of `const char[]`.
> These variables are used not only inside the matcher function but also in the check() function.
> 
I didn't see that, but they should still be `StringRef` as type.


================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:6
+void voidBadForLoop() {
+  for (int i = 0; i < size(); ++i) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has a narrower type ('int') than the type ('long') of termination condition [bugprone-too-small-loop-variable]
----------------
ztamas wrote:
> JonasToth wrote:
> > please add tests where the rhs is a literal.
> Do you mean tests like voidForLoopWithLiteralCond()?
> Is it worth to add more tests like that?
I didn't see it. In principle yes, but i would like to see a test with a bigger number then iterateable (maybe there is a frontend warning for that?). If there is no warning, this should definitely be implemented here (possible follow up) or even become a frontend warning.


================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:152
+void voidForLoopWithEnumCond() {
+  for (short i = eSizeType::START; i < eSizeType::END; ++i) {
+  } // no warning
----------------
ztamas wrote:
> JonasToth wrote:
> > It is possible to specify the underlying type of an `enum`.
> > For the case `enum eSizeType2 : int;` the problem does occur as well. I think especially this case is tricky to spot manually and should be handled too. What do you think?
> Hmm, it can be handled I think. However, I'm not sure how often it is, that an enum has an item value bigger than 32767 (short) or 65535 (unsigned short) or another even bigger value.
> For now, I think it's better to just ignore these cases to avoid false positives and keep the first version of the check simple. The scope can be extended anytime later I think.
`enum` values can become very big for flag-enums

```
enum Foo {
  value1 = 1 << 1;
// ...
  value 24 = 1 << 24;
};
```

You should still add a test for a `enum` with specified underlying type to ensure, there is no noise.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974