[clang-tidy] Add `AllowIteratorDecrementIncrement` option to cppcoreguidelines-pro-bounds-pointer-arithmetic

[carlosgalvezp]

The check currently warns at this type of code:

while(it != end)
{
   // ...
   ++it;  // warning
}

This is technically correct as per the guidelines. However, this pattern is widespread and the warning creates a fair amount of noise. Noise which may lead to people disabling the check altogether, which leads to missing warnings on more serious types of pointer arithmetic.

Thus, I propose to introduce an option AllowIteratorDecrementIncrement (or similar), off-by-default, to be able to relax the check a bit and allow for these use cases and reduce noise. It can also help enabling the check in different steps, from the more serious cases to the less serious ones.

As an example, the corresponding rule in MISRA C++:2008 does allow these operations as an exception to the rule.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Carlos Galvez (carlosgalvezp)

The check currently warns at this type of code:

while(it != end)
{
   // ...
   ++it;  // warning
}

This is technically correct as per the guidelines. However, this pattern is widespread and the warning creates a fair amount of noise. Noise which may lead to people disabling the check altogether, which leads to missing warnings on more serious types of pointer arithmetic.

Thus, I propose to introduce an option AllowIteratorDecrementIncrement (or similar), off-by-default, to be able to relax the check a bit and allow for these use cases and reduce noise. It can also help enabling the check in different steps, from the more serious cases to the less serious ones.

As an example, the corresponding rule in MISRA C++:2008 does allow these operations as an exception to the rule.

[carlosgalvezp]

(The noise became more pronounced after this patch, which now (rightly) highlights issues also in template functions and type aliases).

[vbvictor]

It could be hard to spot these patterns because desugared type of an iterator can be raw pointer and the check simply wouldn't spot the difference between real raw-pointers and desugared-iterator-raw-pointer.

Instead user can mitigate it by using std::next: https://godbolt.org/z/f7aWW4zeK

[vbvictor]

Also, I guess it only happens with std::array which has raw-pointer as iterator (with std::vector there were no issues https://godbolt.org/z/cfTncdKdx), maybe we could suppress for it specifically (like we did in some other issue that I don't exactly remember)

[carlosgalvezp]

It could be hard to spot these patterns

I'm not sure I follow, why would it need to make that difference? My proposal would be to ignore these operators on either raw-pointers or desugared-iterator-raw-pointers.

Yes, std::next can be used as a workaround most of the times, but I found another one case that I found particularly annoying:

for (auto it = x.begin(); it != x.end(); ++it)
{
  // Assume complex logic such that range-based for loop is not appropriate here
}

In this case, one would need to replace it with:

for (auto it = x.begin(); it != x.end(); std::advance(it, 1))

Or with a while loop like above, which arguably goes in detriment to readability (and thus increases the chances of bugs).

I guess it only happens with std::array

Will check!

[vbvictor]

I'm not sure I follow, why would it need to make that difference? My proposal would be to ignore these operators on either raw-pointers or desugared-iterator-raw-pointers.

Oh, I initially thought we should ignore only iterator types (option name say so), not any raw-pointers. With any raw pointers the task is easier - just math of we are in loop, which has it != end condition pattern, so I worry less about implemenation.

[carlosgalvezp]

Yes I meant both raw pointers and iterators, sorry if the proposed name suggested otherwise :)