Part A of clang-tidy bugprone-implicit-widening-of-multiplication-result

[gnawme]

Part A of support for the clang-tidy check bugprone-implicit-widening-of-multiplication-result.

As with PR #5665, this check will be implemented over several PRs to keep them to a reasonable size.

[mvieth]

Thanks for the pull request, however I am unsure how much benefit the proposed changes bring. As far as I can tell, the added casts only make the widening explicit, but do not prevent any overflow? So essentially, by activating the implicit-widening clang-tidy check and adding the casts, we would first activate the warning, then silence it, or did I misunderstand something? Don't get me wrong, the check itself seems quite useful, and we indeed saw this kind of overflow in the past, when users operated on millions or even billions of points, and each point had 32+ byte of information. So I think a better approach would be to identify where an overflow could happen, and then "perform the multiplication in a wider type", as the clang-tidy doc suggests.

[gnawme]

Thanks for the pull request, however I am unsure how much benefit the proposed changes bring. As far as I can tell, the added casts only make the widening explicit, but do not prevent any overflow? So essentially, by activating the implicit-widening clang-tidy check and adding the casts, we would first activate the warning, then silence it, or did I misunderstand something? Don't get me wrong, the check itself seems quite useful, and we indeed saw this kind of overflow in the past, when users operated on millions or even billions of points, and each point had 32+ byte of information. So I think a better approach would be to identify where an overflow could happen, and then "perform the multiplication in a wider type", as the clang-tidy doc suggests.

I believe that the casting is to prevent the product from overflowing, although it's true that there's nothing to prevent the product from overflowing the size_type in resize calls (which is where the bulk of the changes appear), although I imagine that the container would throw in that case.

This check does flag places where implicit widening could cause issues, so the developer could take measures other than simply casting to a wider type (unless that's all that is necessary). (CI doesn't employ the -fix flag in the clang-tidy checks, so it would be up to the developer to apply any necessary fixes.)

What approach would you like to take with this check?

[mvieth]

I believe that the casting is to prevent the product from overflowing,

But if the multiplication is performed first, then the casting, and the overflow would happen during the multiplication, how would the casting help? The cast would only change the datatype of the wrong (overflown) result. Without the cast, the datatype would still be changed, but implicitly. As I understand the clang-tidy doc, adding the static_cast would only "silence the code by making widening explicit", but not fix anything.

although it's true that there's nothing to prevent the product from overflowing the size_type in resize calls (which is where the bulk of the changes appear), although I imagine that the container would throw in that case.

I think overflow of a size type (std::size_t) is very unlikely in PCL, and we can disregard that.

This check does flag places where implicit widening could cause issues, so the developer could take measures other than simply casting to a wider type (unless that's all that is necessary). (CI doesn't employ the -fix flag in the clang-tidy checks, so it would be up to the developer to apply any necessary fixes.)

What approach would you like to take with this check?

I think it is necessary to decide that case-by-case. In most cases, I would change the datatype of the factors, either by casting the factors directly in the product, or by changing the type of the variables, if two variable are multiplied.