llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.11k stars 12.01k forks source link

[bugprone-misplaced-widening-cast] confusing use of "ineffective" in error message #48205

Open a4738398-384b-4ea9-bc79-f580fbdd3992 opened 3 years ago

a4738398-384b-4ea9-bc79-f580fbdd3992 commented 3 years ago
Bugzilla Link 48861
Version unspecified
OS Linux
CC @Xazax-hun

Extended Description

clang-tidy running on the following snippet

void g(int) { } void g(unsigned long) { } void f(int a, int b) { g(static_cast(a + b)); }

complains

warning: either cast from 'int' to 'unsigned long' is ineffective, or there is loss of precision before the conversion [bugprone-misplaced-widening-cast] g(static_cast(a + b)); ^

As far as I can tell, "ineffective cast" is not a standard term, and I can't guess what it means. The documentation at https://clang.llvm.org/extra/clang-tidy/checks/bugprone-misplaced-widening-cast.html does not give any further explanation.

I know based on the source of the arguments that there is no loss of precision in this expression, so this implies that the cast is "ineffective". But removing the cast changes the meaning of the program (a different overload of g is selected), so "ineffective" apparently does not mean "having no effect". It is not clear what action clang-tidy is requesting in this case.

a4738398-384b-4ea9-bc79-f580fbdd3992 commented 3 years ago

Certainly there are ways to silence the warning, but this:

If there is no loss of precision then the cast can be removed or you can explicitly cast to int instead.

and the overall claim of ineffectiveness, is false. I can't remove the cast, and I can't cast to int instead. The cast is guaranteed to not change the value, but there is more to c++ data than that.

I'm not disagreeing that the check is useful and correct, I just think that the warning message and documentation are wrong.

whisperity commented 3 years ago

"Ineffective" I believe was supposed to mean "has no effect". Agreed, this isn't a C++ standard defined term. As the documentation says:

If there is no loss of precision then the cast can be removed or you can explicitly cast to int instead.

If you want to avoid loss of precision then put the cast in a proper location, for instance:

return (long)x * 1000;

You are calculating an (signed) int result first (as a + b is still just int) and then forcing it to be cast to unsigned long, that's why the check reports.

What happens if you write:

g( static_cast(a) + b );

It should select the right overload and make the warning go away.