[PATCH] Loss of Sign Checker

[Soumitra Chatterjee]

On 23-Jun-2015 02:27 PM, Daniel Marjamäki wrote:
> I recommend a more specific name than LossOfSignChecker unless you want it to have more warnings later. Right now, a name such as LossOfSignInAssignmentChecker.cpp would be better imho.
I don't have immediate plans to add more warnings on this, but there 
definitely are many more cases where we can lose the sign and 
potentially cause runtime failures.

That said, I can rename it as you suggest since I do not have immediate 
plans to address more cases.

> Sure there can be issues with loss of sign. If we can warn about mistakes that is awesome!
>
> However personally I think it seems noisy to warn about:
>
>    unsigned char uc1 = -1;
>
> As I see it, it is quite clear that the developer wants to assign -1 to uc1 and that a cast is expected. On a normal machine the uc1 would get the value 255 and this is likely expected.
Sure, but wouldn't it be better to expect an explicit cast if the 
developer really meant what he wrote?

> Have you tried this checker on some real projects to see if the results are good?
No, not really.

However, I hit this issue when moving a large software from x86 to 
AArch64, where unfortunately, the 'sign' of a plain char changes, 
causing us to lose days of effort in narrowing down the issue.

For example, the following representative program that works perfectly 
on x86 would fail silently on AArch64:

      1  #include <stdio.h>
      2
      3  const char data[] = { -1 };
      4
      5  int main() {
      6     int value = data[0];
      7     if (value != -1) {
      8        printf("failed! value = %d\n", value);
      9        return 1;
     10     }
     11     return 0;
     12  }

Do you think narrowing down the checker to the above singular case would 
be more appropriate? I would still think that while the 'unsigned char' 
case you point out may be a grey area, an "integer" assignment with the 
wrong sign would certainly not be a good programming practice.

Thanks,
Soumitra

>
>
> http://reviews.llvm.org/D10634
>
> EMAIL PREFERENCES
>    http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>