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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits