[clang]Fix warning for signed conversion on LP64

[yaxunl]

Summary

Currently clang emits warning with -Wconversion for the following code
on LP64 system e.g. x86_64-unknown-linux-gnu:

long foo(long x) {
  return 1LL<<x;
}

warning: implicit conversion changes signedness: 'long long' to 'long' [-Wsign-conversion]

return 1ll << x;
~~~~~~ ~~~~^~~~

This does not make sense since all operands are signed.

This patch fixes that to match -m32 and GCC behaviour.

[shafik]

If I look at the clang docs for Wconversion I see it includes -Wshorten-64-to-32 which I believe this is a case of. I think maybe the warning needs a better warning for this case?

[yaxunl]

In D144011#4126553, @shafik wrote:

If I look at the clang docs for Wconversion I see it includes -Wshorten-64-to-32 which I believe this is a case of. I think maybe the warning needs a better warning for this case?

This is done for x86_64 on linux, where long long and long are both 64bit. https://godbolt.org/z/hd3qWW5jj

[MaskRay]

I think it makes sense for -Wsign-conversion to not warn for this LP64 case, like we don't emit a warning for -m32. I do not know whether we need another diagnostic like -Wshorten-64-to-32 for this case, but am inclined to no.

I wonder whether the newly added condition can be merged with the following condition:

if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
    ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
     (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
      LikelySourceRange.Width == TargetRange.Width))) {
  if (S.SourceMgr.isInSystemMacro(CC))

[yaxunl]

In D144011#4126853, @MaskRay wrote:

I think it makes sense for -Wsign-conversion to not warn for this LP64 case, like we don't emit a warning for -m32. I do not know whether we need another diagnostic like -Wshorten-64-to-32 for this case, but am inclined to no.

I wonder whether the newly added condition can be merged with the following condition:

if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
    ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
     (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
      LikelySourceRange.Width == TargetRange.Width))) {
  if (S.SourceMgr.isInSystemMacro(CC))

Yes. Will do.

[yaxunl]

revised by Fanrui's comments

[MaskRay]

Thanks!

clang/lib/Sema/SemaChecking.cpp

14296

Consider adding an example when this condition triggers to help understanding, e.g. long t2 = 1LL << x; on LP64 systems.

[MaskRay]

The description needs to adjusted to say that this confusing warning is for LP64 systems.
It makes sense to match -m32 and GCC. If someone thinks a warning is useful, that can be a separate change.

[yaxunl]

clang/lib/Sema/SemaChecking.cpp

14296

will do when committing

clang/lib/Sema/SemaChecking.cpp

 
   if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
       ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
        (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
         LikelySourceRange.Width == TargetRange.Width))) {
     if (S.SourceMgr.isInSystemMacro(CC))
       return;
 
+    if (SourceBT && SourceBT->isInteger() && TargetBT &&
+        TargetBT->isInteger() &&
+        Source->isSignedIntegerType() == Target->isSignedIntegerType()) {
+      return;
+    }
+
     unsigned DiagID = diag::warn_impcast_integer_sign;
 
     // Traditionally, gcc has warned about this under -Wsign-compare.
     // We also want to warn about it in -Wconversion.
     // So if -Wconversion is off, use a completely identical diagnostic
     // in the sign-compare group.
     // The conditional-checking code will
     if (ICContext) {

clang/test/Sema/sign-conversion.c

 // RUN: %clang_cc1 -fsyntax-only -verify -Wsign-conversion %s
 
 // PR9345: make a subgroup of -Wconversion for signedness changes
 
 void test(int x) {
   unsigned t0 = x; // expected-warning {{implicit conversion changes signedness}}
   unsigned t1 = (t0 == 5 ? x : 0); // expected-warning {{operand of ? changes signedness}}
+
+  // Clang has special treatment for left shift of literal '1'.
+  // Make sure there is no diagnostics.
+  long t2 = 1LL << x;
 }