[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

Wed Mar 27 23:34:55 PDT 2019

ztamas marked an inline comment as done.
ztamas added a comment.

In D59870#1444645 <https://reviews.llvm.org/D59870#1444645>, @JonasToth wrote:

> I think in general this is a good direction. What do you think about other heuristics?
>  E.g. one could say that a vector will not contain more then X GB or similar. That is probably more complicated to figure out though.

Yes, it would be very complicated to find out the memory size of the container used in the code, I think. The used size method is not always the size() function. In llvm code I see also std::string::length() and array_lengthof(). Also a code might use a non-standard container, which might have different method for getting the size of the container which makes it hard to create a matcher for this and find out which container I need to get it's size.

Also the check can catch loops which are not related to containers, so a container related option would have no meening in these cases.

I think it's the easiest way to specify the bits of the ineteger type to limit the catches. In real life, I met with this overflow / infinite loop problem with 16-bit short type, so I think the real use cases are 8 and 16 bit integers. It seems intuitive to me to use the size of the loop variable's type to separate those catches which can lead broken functionality in practice from those use cases which are just integer incompatibilities.

================
Comment at: docs/ReleaseNotes.rst:122

+- The :bugprone-too-small-loop-variable
+  <clang-tidy/checks/bugprone-too-small-loop-variable>` now supports
----------------
Eugene.Zelenko wrote:
> :doc prefix and ` is missing. Please also rebase patch from trunk.
I'm a bit confused now about the code repositories, because of the recent changes. Which repo should I clone and rebase my commit?
Is this one the trunk?: http://llvm.org/svn/llvm-project/llvm/trunk

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59870/new/

https://reviews.llvm.org/D59870