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

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 16 inline comments as done.
ztamas added inline comments.


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";
----------------
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.



================
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]
----------------
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?


================
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:116
+
+// TODO: handle those cases when the loop condition contains a floating point expression
+void voidDoubleForCond() {
----------------
whisperity wrote:
> (Misc: You might want to check out D52730 for floats later down the line.)
In the meantime, I thought this float comparison is not a real use case. It just came in my mind while I was trying to find out test cases. So I just removed it. The referenced conversion check might catch it anyway. 


================
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
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974