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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

last nits from my side (for now :)).
If the other reviews could take a look at it as well, would be great.
I am uncertain about the english in some comments @aaron.ballman finds all these language bugs ;)



================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:123
+      return calcPositiveBits(Context, RHSEType);
+    else
+      return std::max(calcPositiveBits(Context, LHSEType),
----------------
please no else after return


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:126
+                      calcPositiveBits(Context, RHSEType));
+  } else
+    return calcPositiveBits(Context, CondExprType);
----------------
same


================
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142
+  if (LoopVar->getType() != LoopIncrement->getType())
+    return; // We matched the loop variable incorrectly
+
----------------
ztamas wrote:
> ztamas wrote:
> > JonasToth wrote:
> > > Does this try to ensure a precondition? Then it should become an assertion instead.
> > > Please adjust the comment like above (punctuation, position)
> > It's not an assumed precondition. This `if` handles the case when LoopVarMatcher is not fitted with the actual loop variable. That's why the IncrementMatcher is there so we can check whether we found the loop variable.
> > See voidForLoopReverseCond() test case which hits this `if` branch.
> > I did not find a solution to handle this check inside the matcher.
> voidForLoopReverseCond()  was renamed to voidForLoopCondImplicitCast() in the mean time.
Ok, I can't think of a solution of head right now as well. It's fine to leave as is.


================
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10
+
+  .. code-block:: c++
+
----------------
ztamas wrote:
> JonasToth wrote:
> > the `.. code-block:: c++` is usually not indended, only the code itself.
> Hmm, I copied this from somewhere. It might be a good idea to make this consistent across all the *.rst files. See bugprone-suspicious-semicolon.rst or bugprone-use-after-move.rst for example.
True, but nobody want's to do the documentation work :D


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974