Page MenuHomePhabricator

[clang-tidy] don't warn about implicit widening casts in function calls
AbandonedPublic

Authored by danielmarjamaki on Mar 17 2017, 1:01 PM.

Details

Reviewers
alexfh
xazax.hun
Summary

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=32246

To avoid spurious warnings, clang-tidy should not warn about misplaced widening casts for implicit casts in function calls.

Diff Detail

Repository
rL LLVM

Event Timeline

In my opinion, we should stop warning about all implicit casts.

Take for instance:

long l1;
if (condition)
  l1 = n << 8;  // <- implicit cast
else
  l1 = ~0L;

That is fine. Nothing suspicious. Just because the destination variable is long doesn't have to mean the result is long. If we want to warn I would say that valueflow analysis should be used to see if there is truncation.

The original idea was that we would warn if the user tried to cast the result but did that wrong. I don't feel that this is the idea of this checker anymore.

xazax.hun edited edge metadata.Mar 18 2017, 2:33 AM

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

alexfh edited edge metadata.Mar 18 2017, 2:38 AM

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

Good point, I'm convinced now.

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

Actually, the diagnostics about implicit casts here might be useful (but maybe in a separate check). I have to look again at https://reviews.llvm.org/D17987.

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

Actually, the diagnostics about implicit casts here might be useful (but maybe in a separate check). I have to look again at https://reviews.llvm.org/D17987.

there can definitely be bugs when there are such implicit casts.

but this checking has no precision at all. I am against that we don't care about precision.

adding casts to silence such warnings are dangerous too. I have seen for instance in Clang repo when there is "loss of sign" warning and the developer fix that by casting for instance a "size_t" to "int" and then there is logically loss of precision.

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

I agree..

Just want to advertise the analyzer ConversionChecker also in case you didn't know about it. That is supposed to be a precise checker for loss of precision and loss of sign. It does not detect this loss of precision in implicit casts but I would like that is taken care of.

Remove warnings for implicit casts.

I believe there is pointless code in relativeIntSizes etc. If there is for instance "a+b" then the result can't be a char type.

static int relativeIntSizes(BuiltinType::Kind Kind) {
  switch (Kind) {
  case BuiltinType::UChar:
    return 1;
  case BuiltinType::SChar:
    return 1;
  case BuiltinType::Char_U:
    return 1;
  case BuiltinType::Char_S:
    return 1;
  case BuiltinType::UShort:
    return 2;
  case BuiltinType::Short:
    return 2;
  case BuiltinType::UInt:
    return 3;
  case BuiltinType::Int:
    return 3;
  case BuiltinType::ULong:
    return 4;
  case BuiltinType::Long:
    return 4;
  case BuiltinType::ULongLong:
    return 5;
  case BuiltinType::LongLong:
    return 5;
  case BuiltinType::UInt128:
    return 6;
  case BuiltinType::Int128:
    return 6;
  default:
    return 0;
  }
}
dkrupp added a subscriber: dkrupp.Mar 22 2017, 3:09 AM

Hi!

There is an option to disable the checking of widening casts. It is enabled by default. You can disable it any time. Or, if you find too much false positives, we can discuss about setting this option to disabled as default.

I am convinced that checking implicit widening casts are also necessary. We should probably change the error message in the implicit case from "misplaced" to "missing", and maybe also rename the checker itself. Separating it to two different checkers, which are almost copy of each other is huge code duplication.

Hi!

There is an option to disable the checking of widening casts. It is enabled by default. You can disable it any time. Or, if you find too much false positives, we can discuss about setting this option to disabled as default.

I am convinced that checking implicit widening casts are also necessary. We should probably change the error message in the implicit case from "misplaced" to "missing", and maybe also rename the checker itself. Separating it to two different checkers, which are almost copy of each other is huge code duplication.

It would help to disable it by default and changing the message. But also I believe it's philosophically different to the original checker.

I would say that your logic is more philosophically similar to Wconversion. Could it be added there instead?

Did you try this check on real code? Do you think there is a problem that should be fixed here?

void foo(unsigned int N) {
  long L;
  if (N<10)
    L = N << 8;
  ...

I am assuming that such code is not uncommon. If you don't think there is a problem in that code then I would personally suggest that you update the ConversionChecker in the analyzer instead.

I do believe that a warning about that code is a false positive.

The idea with the misc-misplaced-widening-cast is that if the developer writes such code:

void foo(unsigned int N) {
  long L;
  if (N<10)
    L = (long)(N << 8);
  ...

Then there is a message "either cast is misplaced or there is truncation". In this case the cast is misplaced and it can be removed.

Well.. feel free to provide an alternative fix. If the message is more specific and it must be enabled explicitly by an option then maybe it's good enough for me.

I would recommend that this is either fixed soon or that we commit my changes so it can be implemented more properly later. Right now users will see false positives.

OK, please create a patch where implicit casts are disabled by default. You can also find a new name for the checker.

Or I can do it for you if you wish.

Or I can do it for you if you wish.

yes please