[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
Jonas Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 5 14:08:01 PST 2018
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);
+ 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);
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142
+ if (LoopVar->getType() != LoopIncrement->getType())
+ return; // We matched the loop variable incorrectly
> 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++
> 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
rCTE Clang Tools Extra
More information about the cfe-commits