[PATCH] D144011: [clang]Fix warning for signed conversion on LP64

[Yaxun Liu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Closed by commit rG8cda128c1eff: [clang]Fix warning for signed conversion on 
LP64 (authored by yaxunl).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D144011?vs=497703=499215#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/sign-conversion.c


Index: clang/test/Sema/sign-conversion.c
===
--- clang/test/Sema/sign-conversion.c
+++ clang/test/Sema/sign-conversion.c
@@ -1,8 +1,14 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify 
-Wsign-conversion %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -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;
 }
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -14316,6 +14316,12 @@
 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.


Index: clang/test/Sema/sign-conversion.c
===
--- clang/test/Sema/sign-conversion.c
+++ clang/test/Sema/sign-conversion.c
@@ -1,8 +1,14 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -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;
 }
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -14316,6 +14316,12 @@
 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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion on LP64

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:14296
 
+if (SourceBT && SourceBT->isInteger() && TargetBT &&
+TargetBT->isInteger() &&

MaskRay wrote:
> Consider adding an example when this condition triggers to help 
> understanding, e.g. `long t2 = 1LL << x;` on LP64 systems.
will do when committing


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Thanks!




Comment at: clang/lib/Sema/SemaChecking.cpp:14296
 
+if (SourceBT && SourceBT->isInteger() && TargetBT &&
+TargetBT->isInteger() &&

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 497703.
yaxunl added a comment.

revised by Fanrui's comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/sign-conversion.c


Index: clang/test/Sema/sign-conversion.c
===
--- clang/test/Sema/sign-conversion.c
+++ clang/test/Sema/sign-conversion.c
@@ -5,4 +5,8 @@
 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;
 }
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -14293,6 +14293,12 @@
 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.


Index: clang/test/Sema/sign-conversion.c
===
--- clang/test/Sema/sign-conversion.c
+++ clang/test/Sema/sign-conversion.c
@@ -5,4 +5,8 @@
 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;
 }
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -14293,6 +14293,12 @@
 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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

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(Target) || !isa(Source)) &&
>   ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
>(!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
> LikelySourceRange.Width == TargetRange.Width))) {
> if (S.SourceMgr.isInSystemMacro(CC))

Yes. Will do.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I think it makes sense for `-Wsign-conversion` to not warn for this case. I do 
not know whether we need another diagnostic like `-Wshorten-64-to-32` for this 
case, but am inclined to no.

`-m32` doesn't emit a warning. I wonder whether the newly added condition can 
be merged with the following condition:

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144011/new/

https://reviews.llvm.org/D144011

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144011: [clang]Fix warning for signed conversion

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added reviewers: tra, rsmith, MaskRay, rtrieu.
Herald added a project: All.
yaxunl requested review of this revision.

Currently clang emits warning with -Wconversion for the following code:

  long foo(long x) {
return 1LLisInteger() && TargetBT &&
+TargetBT->isInteger()) {
+  return;
+}
+
 // Fall through for non-constants to give a sign conversion warning.
   }
 


Index: clang/test/Sema/sign-conversion.c
===
--- clang/test/Sema/sign-conversion.c
+++ clang/test/Sema/sign-conversion.c
@@ -5,4 +5,8 @@
 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;
 }
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -14281,6 +14281,11 @@
   }
 }
 
+if (SourceBT && SourceBT->isInteger() && TargetBT &&
+TargetBT->isInteger()) {
+  return;
+}
+
 // Fall through for non-constants to give a sign conversion warning.
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits