[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 544732.
xgupta added a comment.

minor update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,25 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types 
with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if (TautologicalTypeCompare &&
+  (LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical) &&
+  !S.Context.hasSameType(Constant->getType(), Other->getType())) {
+return false;
+  }
+
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,25 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if (TautologicalTypeCompare &&
+  (LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical) &&
+  !S.Context.hasSameType(Constant->getType(), Other->getType())) {
+return false;
+  }
+
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 544728.
xgupta added a comment.

updated code as per the comment of @cor3ntin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp
  clang/test/SemaCXX/bool-compare.cpp


Index: clang/test/SemaCXX/bool-compare.cpp
===
--- clang/test/SemaCXX/bool-compare.cpp
+++ clang/test/SemaCXX/bool-compare.cpp
@@ -8,13 +8,13 @@
   if(b > true){} // expected-warning {{comparison of true with expression 
of type 'bool' is always false}}
   if(b < true){} // no warning
   if(b >= true)   {} // no warning
-  if(b <= true)   {} // expected-warning {{comparison of true with expression 
of type 'bool' is always true}}
+  if(b <= true)   {} // expected-warning {{result of comparison of true with 
expression of type 'bool' is always true}}
   if(b == true)   {} // no warning
   if(b != true)   {} // no warning
 
   if(b > false)   {} // no warning
-  if(b < false)   {} // expected-warning {{comparison of false with expression 
of type 'bool' is always false}}
-  if(b >= false)  {} // expected-warning {{comparison of false with expression 
of type 'bool' is always true}}
+  if(b < false)   {} // expected-warning {{result of comparison of false with 
expression of type 'bool' is always false}}
+  if(b >= false)  {} // expected-warning {{result of comparison of false with 
expression of type 'bool' is always true}}
   if(b <= false)  {} // no warning
   if(b == false)  {} // no warning
   if(b != false)  {} // no warning
Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,25 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types 
with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if (TautologicalTypeCompare &&
+  (LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical) &&
+  !S.Context.hasSameType(Constant->getType(), Other->getType())) {
+return false;
+  }
+
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/SemaCXX/bool-compare.cpp
===
--- clang/test/SemaCXX/bool-compare.cpp
+++ clang/test/SemaCXX/bool-compare.cpp
@@ -8,13 +8,13 @@
   if(b > true){} // expected-warning {{comparison of true with expression of type 'bool' is always false}}
   if(b < true){} // no warning
   if(b >= true)   {} // no warning
-  if(b <= true)   {} // expected-warning {{comparison of true with expression of type 'bool' is always true}}
+  if(b <= true)   {} // expected-warning {{result of comparison of true with expression of type 'bool' is always true}}
   if(b == true)   {} // no warning
   if(b != true)   {} // no warning
 
   if(b > false)   {} // no warning
-  if(b < false)   {} // expected-warning {{comparison of false with expression of type 'bool' is always false}}
-  if(b >= false)  {} // expected-warning {{comparison of false with expression of type 'bool' is always true}}
+  if(b < false)   {} // expected-warning {{result of comparison of false with expression of type 'bool' is always false}}
+  if(b >= false)  {} // expected-warning {{result of comparison of false with expression of type 'bool' is always true}}
   if(b <= false)  {} // no warning
   if(b == false)  {} // no warning
   if(b != false)  {} // no warning
Index: 

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:13807-13815
+  // Don't warn if the comparison involves integral or floating-point types 
with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if ((LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical)) {

This is what I had in mind. I haven't tested it though


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4535941 , @aaron.ballman 
wrote:

> In D155457#4526168 , @xgupta wrote:
>
>> In D155457#4523388 , @cor3ntin 
>> wrote:
>>
>>> I'm not sure I understand the motivation for this change. Sure, people do 
>>> that but they also might do the same thing for ssize_t, intmax_t, or to 
>>> compare int to int32_t.
>>> I think a better heuristic would be to not emit a warning for any integral 
>>> (and floating point?) type that have the same canonical types (but we 
>>> probably still want one if their non-canonical type if the same)
>>
>> I am not sure but are you expecting these changes -
>>
>>   // Don't warn if the comparison involves integral or floating-point types 
>> with the same canonical types.
>>   QualType LHSCanonical = Constant->getType().getCanonicalType();
>>   QualType RHSCanonical = Other->getType().getCanonicalType();
>>   if ((LHSCanonical->isIntegralOrEnumerationType() || 
>> LHSCanonical->isFloatingType()) &&
>>   S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
>> return false;
>>   }
>>
>> This will silence a lot of warnings and a total 5 test case fails.
>
> Can you share some examples of what test cases start failing with that 
> approach? What you have above matches what I think @cor3ntin was asking for 
> and does seem like a pretty reasonable way to silence false positives.

Sure, updated three test cases in the patch and list the other two here -

  FAIL: Clang :: Sema/tautological-constant-compare.c (865 of 18988)
   TEST 'Clang :: Sema/tautological-constant-compare.c' 
FAILED 
  
  error: 'warning' diagnostics expected but not seen: 
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 560 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:568):
 comparison of 3-bit signed value < 4 is always true
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 574 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:585):
 comparison of 8-bit unsigned value < 0 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 593 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:595):
 comparison of 2-bit unsigned value > 3 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 603: result of comparison 'int' > 2147483647 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 608 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:610):
 comparison of 15-bit unsigned value > 32767 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 614 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:616):
 comparison of 6-bit signed value > 31 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 621 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:637):
 comparison of 4-bit signed value < -8 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 623 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:638):
 comparison of 4-bit signed value > 7 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 628 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:639):
 comparison of 5-bit signed value < -16 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 629 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:640):
 comparison of 5-bit signed value > 15 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 632 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:641):
 comparison of 4-bit signed value > 7 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 633 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:642):
 comparison of 4-bit signed value < -8 is always false
File 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c 
Line 635 (directive at 
/home/shivam/.llvm/llvm-project/clang/test/Sema/tautological-constant-compare.c:643):
 comparison of 5-bit 

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 544622.
xgupta added a comment.

Update as per comment of @cor3ntin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/atomic-compare.c
  clang/test/Sema/tautological-objc-bool-compare.m
  clang/test/Sema/type-limit-compare.cpp
  clang/test/SemaCXX/bool-compare.cpp

Index: clang/test/SemaCXX/bool-compare.cpp
===
--- clang/test/SemaCXX/bool-compare.cpp
+++ clang/test/SemaCXX/bool-compare.cpp
@@ -5,16 +5,16 @@
 
   bool a,b;
 
-  if(b > true){} // expected-warning {{comparison of true with expression of type 'bool' is always false}}
+  if(b > true){}
   if(b < true){} // no warning
   if(b >= true)   {} // no warning
-  if(b <= true)   {} // expected-warning {{comparison of true with expression of type 'bool' is always true}}
+  if(b <= true)   {}
   if(b == true)   {} // no warning
   if(b != true)   {} // no warning
 
   if(b > false)   {} // no warning
-  if(b < false)   {} // expected-warning {{comparison of false with expression of type 'bool' is always false}}
-  if(b >= false)  {} // expected-warning {{comparison of false with expression of type 'bool' is always true}}
+  if(b < false)   {}
+  if(b >= false)  {}
   if(b <= false)  {} // no warning
   if(b == false)  {} // no warning
   if(b != false)  {} // no warning
Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/test/Sema/tautological-objc-bool-compare.m
===
--- clang/test/Sema/tautological-objc-bool-compare.m
+++ clang/test/Sema/tautological-objc-bool-compare.m
@@ -15,10 +15,10 @@
   r = B >= 0; // expected-warning {{result of comparison of constant 0 with expression of type 'BOOL' is always true, as the only well defined values for 'BOOL' are YES and NO}}
   r = B <= 0;
 
-  r = B > YES; // expected-warning {{result of comparison of constant YES with expression of type 'BOOL' is always false, as the only well defined values for 'BOOL' are YES and NO}}
+  r = B > YES;
   r = B > NO;
-  r = B < NO; // expected-warning {{result of comparison of constant NO with expression of type 'BOOL' is always false, as the only well defined values for 'BOOL' are YES and NO}}
+  r = B < NO;
   r = B < YES;
-  r = B >= NO; // expected-warning {{result of comparison of constant NO with expression of type 'BOOL' is always true, as the only well defined values for 'BOOL' are YES and NO}}
+  r = B >= NO;
   r = B <= NO;
 }
Index: clang/test/Sema/atomic-compare.c
===
--- clang/test/Sema/atomic-compare.c
+++ clang/test/Sema/atomic-compare.c
@@ -14,10 +14,10 @@
   if (a > 2) {} // no warning
 
   if (!a > 0) {}  // no warning
-  if (!a > 1) {} // expected-warning {{comparison of constant 1 with boolean expression is always false}}
-  if (!a > 2) {} // expected-warning {{comparison of constant 2 with boolean expression is always false}}
+  if (!a > 1) {}
+  if (!a > 2) {}
   if (!a > b) {} // no warning
-  if (!a > -1){} // expected-warning {{comparison of constant -1 with boolean expression is always true}}
+  if (!a > -1){}
 }
 
 typedef _Atomic(int) Ty;
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,16 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves integral or floating-point types with
+  // the same canonical types.
+  QualType LHSCanonical = Constant->getType().getCanonicalType();
+  QualType RHSCanonical = Other->getType().getCanonicalType();
+  if ((LHSCanonical->isIntegralOrEnumerationType() ||
+   LHSCanonical->isFloatingType()) &&
+  S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155457#4526168 , @xgupta wrote:

> In D155457#4523388 , @cor3ntin 
> wrote:
>
>> I'm not sure I understand the motivation for this change. Sure, people do 
>> that but they also might do the same thing for ssize_t, intmax_t, or to 
>> compare int to int32_t.
>> I think a better heuristic would be to not emit a warning for any integral 
>> (and floating point?) type that have the same canonical types (but we 
>> probably still want one if their non-canonical type if the same)
>
> I am not sure but are you expecting these changes -
>
>   // Don't warn if the comparison involves integral or floating-point types 
> with the same canonical types.
>   QualType LHSCanonical = Constant->getType().getCanonicalType();
>   QualType RHSCanonical = Other->getType().getCanonicalType();
>   if ((LHSCanonical->isIntegralOrEnumerationType() || 
> LHSCanonical->isFloatingType()) &&
>   S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
> return false;
>   }
>
> This will silence a lot of warnings and a total 5 test case fails.

Can you share some examples of what test cases start failing with that 
approach? What you have above matches what I think @cor3ntin was asking for and 
does seem like a pretty reasonable way to silence false positives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4523388 , @cor3ntin wrote:

> I'm not sure I understand the motivation for this change. Sure, people do 
> that but they also might do the same thing for ssize_t, intmax_t, or to 
> compare int to int32_t.
> I think a better heuristic would be to not emit a warning for any integral 
> (and floating point?) type that have the same canonical types (but we 
> probably still want one if their non-canonical type if the same)

I am not sure but are you expecting these changes -

  // Don't warn if the comparison involves integral or floating-point types 
with the same canonical types.
  QualType LHSCanonical = Constant->getType().getCanonicalType();
  QualType RHSCanonical = Other->getType().getCanonicalType();
  if ((LHSCanonical->isIntegralOrEnumerationType() || 
LHSCanonical->isFloatingType()) &&
  S.Context.hasSameType(LHSCanonical, RHSCanonical)) {
return false;
  }

This will silence a lot of warnings and a total 5 test case fails.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4523227 , @aaron.ballman 
wrote:

> In D155457#4513812 , @xgupta wrote:
>
>> In D155457#4511652 , 
>> @aaron.ballman wrote:
>>
>>> In the x86 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is true, 
>>> so we test the other expression; because `Size` is greater than 
>>> `__SIZE_MAX__` the function returns false and the second static assertion 
>>> fails as expected.
>>> In the x64 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is false 
>>> (first static assertion fails) so we shouldn't even be evaluating the RHS 
>>> of the `&&` to see if it's tautological because it can't contribute to the 
>>> expression result, right?
>>
>> Yes, I agree with both statements but what is odd in current behavior?
>
> It's a false positive -- the tautological bit is diagnosed but it doesn't 
> contribute to the result of the expression. The first part of the predicate 
> is specifically intended to ensure the second part of the predicate is *not* 
> tautological.

Ok, Is there any modification required in this patch or it fixes that false 
positive?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

I'm not sure I understand the motivation for this change. Sure, people do that 
but they also might do the same thing for ssize_t, intmax_t, or to compare int 
to int32_t.
I think a better heuristic would be to not emit a warning for any integral (and 
floating point?) type that have the same canonical types (but we probably still 
want one if their non-canonical type if the same)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155457#4513812 , @xgupta wrote:

> In D155457#4511652 , @aaron.ballman 
> wrote:
>
>> In the x86 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is true, so 
>> we test the other expression; because `Size` is greater than `__SIZE_MAX__` 
>> the function returns false and the second static assertion fails as expected.
>> In the x64 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is false 
>> (first static assertion fails) so we shouldn't even be evaluating the RHS of 
>> the `&&` to see if it's tautological because it can't contribute to the 
>> expression result, right?
>
> Yes, I agree with both statements but what is odd in current behavior?

It's a false positive -- the tautological bit is diagnosed but it doesn't 
contribute to the result of the expression. The first part of the predicate is 
specifically intended to ensure the second part of the predicate is *not* 
tautological.

> It seems to me uint64_t is unsigned long in , not unsigned long long 
> so the new test added in this patch (with `typedef unsigned long uint64_t`) 
> is now passing.
>
> Behavior when the patch is applied-
>
>   $ ./bin/clang++ -Wall -fsyntax-only test.cpp -m64  -Wtype-limits 
>   test.cpp:8:11: warning: result of comparison 'uint64_t' (aka 'unsigned long 
> long') > 18446744073709551615 is always false 
> [-Wtautological-type-limit-compare]
>   8 |  Size > static_cast(__SIZE_MAX__)) // no-warning
> |   ^ ~~~
>   test.cpp:13:15: error: static assertion failed due to requirement 
> 'sizeof(unsigned long) < sizeof(unsigned long long)': 
>  13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
> |   ^~
>   test.cpp:13:35: note: expression evaluates to '8 < 8'
>  13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
> |   ^~
>   1 warning and 1 error generated.
>
>   $ ./bin/clang++ -Wall -fsyntax-only test1.cpp -m64  -Wtype-limits 
>   test1.cpp:12:15: error: static assertion failed due to requirement 
> 'sizeof(unsigned long) < sizeof(unsigned long)': 
>  12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
> |   ^~
>   test1.cpp:12:35: note: expression evaluates to '8 < 8'
>  12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
> |   ^~
>   1 error generated.
>
> where test.cpp using wrapper and test1.cpp is using standard headers.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

Just normal ping, @aaron.ballman, can please take a look, pre-merge checks are 
also passing now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 542846.
xgupta added a comment.

update test for Windows system


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+#if defined(_WIN32)
+typedef unsigned long long uint64_t;
+#else
+typedef unsigned long uint64_t;
+#endif
+
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4511652 , @aaron.ballman 
wrote:

> In the x86 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is true, so 
> we test the other expression; because `Size` is greater than `__SIZE_MAX__` 
> the function returns false and the second static assertion fails as expected.
> In the x64 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is false 
> (first static assertion fails) so we shouldn't even be evaluating the RHS of 
> the `&&` to see if it's tautological because it can't contribute to the 
> expression result, right?

Yes, I agree with both statements but what is odd in current behavior?

It seems to me uint64_t is unsigned long in , not unsigned long long 
so the new test added in this patch (with `typedef unsigned long uint64_t`) is 
now passing.

Behavior when the patch is applied-

  $ ./bin/clang++ -Wall -fsyntax-only test.cpp -m64  -Wtype-limits 
  test.cpp:8:11: warning: result of comparison 'uint64_t' (aka 'unsigned long 
long') > 18446744073709551615 is always false 
[-Wtautological-type-limit-compare]
  8 |  Size > static_cast(__SIZE_MAX__)) // no-warning
|   ^ ~~~
  test.cpp:13:15: error: static assertion failed due to requirement 
'sizeof(unsigned long) < sizeof(unsigned long long)': 
 13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  test.cpp:13:35: note: expression evaluates to '8 < 8'
 13 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  1 warning and 1 error generated.

  $ ./bin/clang++ -Wall -fsyntax-only test1.cpp -m64  -Wtype-limits 
  test1.cpp:12:15: error: static assertion failed due to requirement 
'sizeof(unsigned long) < sizeof(unsigned long)': 
 12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  test1.cpp:12:35: note: expression evaluates to '8 < 8'
 12 | static_assert(sizeof(std::size_t) < sizeof(uint64_t), "");
|   ^~
  1 error generated.

where test.cpp using wrapper and test1.cpp is using standard headers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta updated this revision to Diff 541866.
xgupta added a comment.

Add test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/type-limit-compare.cpp


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+
+typedef unsigned long uint64_t;
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/test/Sema/type-limit-compare.cpp
===
--- /dev/null
+++ clang/test/Sema/type-limit-compare.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
+
+// expected-no-diagnostics
+
+typedef unsigned long uint64_t;
+namespace std {
+using size_t = decltype(sizeof(0));
+} // namespace std
+
+bool func(uint64_t Size) {
+  if (sizeof(std::size_t) < sizeof(uint64_t) &&
+ Size > (uint64_t)(__SIZE_MAX__))
+return false;
+  return true;
+}
+
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13804,6 +13804,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155457#4511446 , @xgupta wrote:

> In D155457#4511392 , @aaron.ballman 
> wrote:
>
>> Whether `size_t` comes from the system header or whether it's manually 
>> deduced from `decltype(sizeof(0))` should make no difference as far as the 
>> frontend is concerned; they should resolve to the same type. Can you explain 
>> the test failure you're seeing in a bit more detail?
>
>
>
>   : 'RUN: at line 1';   /home/shivam/.llvm/llvm-project/build/bin/clang -cc1 
> -internal-isystem /home/shivam/.llvm/llvm-project/build/lib/clang/17/include 
> -nostdsysteminc 
> /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp 
> -fsyntax-only -Wtautological-type-limit-compare -verify
>   --
>   Exit Code: 1
>   
>   Command Output (stderr):
>   --
>   error: 'warning' diagnostics seen but not expected: 
> File 
> /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp Line 
> 12: result of comparison 'uint64_t' (aka 'unsigned long long') > 
> 18446744073709551615 is always false
>   1 error generated.
>
> I think the issue is with  (uint64_t)(SIZE_MAX) vs 
> (uint64_t)std::numeric_limits::max() but not sure.

Something seems odd to me about the current behavior. Consider: 
https://godbolt.org/z/ofezfhaPd

In the x86 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is true, so we 
test the other expression; because `Size` is greater than `__SIZE_MAX__` the 
function returns false and the second static assertion fails as expected.
In the x64 compilation: `sizeof(std::size_t) < sizeof(uint64_t)` is false 
(first static assertion fails) so we shouldn't even be evaluating the RHS of 
the `&&` to see if it's tautological because it can't contribute to the 
expression result, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4511392 , @aaron.ballman 
wrote:

> Whether `size_t` comes from the system header or whether it's manually 
> deduced from `decltype(sizeof(0))` should make no difference as far as the 
> frontend is concerned; they should resolve to the same type. Can you explain 
> the test failure you're seeing in a bit more detail?



  : 'RUN: at line 1';   /home/shivam/.llvm/llvm-project/build/bin/clang -cc1 
-internal-isystem /home/shivam/.llvm/llvm-project/build/lib/clang/17/include 
-nostdsysteminc 
/home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp 
-fsyntax-only -Wtautological-type-limit-compare -verify
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  error: 'warning' diagnostics seen but not expected: 
File /home/shivam/.llvm/llvm-project/clang/test/Sema/type-limit-compare.cpp 
Line 12: result of comparison 'uint64_t' (aka 'unsigned long long') > 
18446744073709551615 is always false
  1 error generated.

I think the issue is with __SIZE_MAX__ vs 
std::numeric_limits::max() but not sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155457#4510202 , @xgupta wrote:

> In D155457#4507124 , @aaron.ballman 
> wrote:
>
>> In D155457#4507107 , @xgupta wrote:
>>
>>> In D155457#4506405 , @xbolva00 
>>> wrote:
>>>
 You should add a testcase which uses “expected no diagnostics”.
>>>
>>> There is some issue writing test
>>>
>>>   $ cat type-limit-compare.cpp
>>>   // RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare 
>>> -verify %s
>>>   // expected-no-diagnostics
>>>   
>>>   #include 
>>>   #include 
>>>   #include 
>>>   
>>>   bool foo(uint64_t Size) {
>>> if (sizeof(std::size_t) < sizeof(uint64_t) &&
>>>Size > 
>>> static_cast(std::numeric_limits::max())) // 
>>> no-warning
>>>   return false;
>>> return true;
>>>   }
>>>
>>> failed with
>>>
>>>   $ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 
>>> 'cstddef' file not found
>>>   4 | #include 
>>> |  ^
>>
>> We typically do not include any system headers (STL or otherwise) as part of 
>> the compiler tests; that would test whatever is found on the test system 
>> instead of a consistent test. Instead, I'd recommend doing:
>>
>>   namespace std {
>>   using size_t = decltype(sizeof(0));
>>   }
>>
>> Similarly, you can replace `uint64_t` with `unsigned long long` and the 
>> `numeric_limits` call with `__SIZE_MAX__`
>
> I see, Thanks,  but there is another thing, writing this way compiler emits a 
> warning as the check to exclude the warning is based on `size_t` so the test 
> case is not passed.

Whether `size_t` comes from the system header or whether it's manually deduced 
from `decltype(sizeof(0))` should make no difference as far as the frontend is 
concerned; they should resolve to the same type. Can you explain the test 
failure you're seeing in a bit more detail?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4507124 , @aaron.ballman 
wrote:

> In D155457#4507107 , @xgupta wrote:
>
>> In D155457#4506405 , @xbolva00 
>> wrote:
>>
>>> You should add a testcase which uses “expected no diagnostics”.
>>
>> There is some issue writing test
>>
>>   $ cat type-limit-compare.cpp
>>   // RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare 
>> -verify %s
>>   // expected-no-diagnostics
>>   
>>   #include 
>>   #include 
>>   #include 
>>   
>>   bool foo(uint64_t Size) {
>> if (sizeof(std::size_t) < sizeof(uint64_t) &&
>>Size > 
>> static_cast(std::numeric_limits::max())) // no-warning
>>   return false;
>> return true;
>>   }
>>
>> failed with
>>
>>   $ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 
>> 'cstddef' file not found
>>   4 | #include 
>> |  ^
>
> We typically do not include any system headers (STL or otherwise) as part of 
> the compiler tests; that would test whatever is found on the test system 
> instead of a consistent test. Instead, I'd recommend doing:
>
>   namespace std {
>   using size_t = decltype(sizeof(0));
>   }
>
> Similarly, you can replace `uint64_t` with `unsigned long long` and the 
> `numeric_limits` call with `__SIZE_MAX__`

I see, Thanks,  but there is another thing, writing this way compiler emits a 
warning as the check to exclude the warning is based on `size_t` so the test 
case is not passed.

  // RUN: %clang_cc1 %s -fsyntax-only -Wtautological-type-limit-compare -verify
  
  // expected-no-diagnostics
  
  typedef unsigned long long uint64_t;
  namespace std {
  using size_t = decltype(sizeof(0));
  } // namespace std
  
  bool func(uint64_t Size) {
if (sizeof(std::size_t) < sizeof(uint64_t) &&
   Size > (uint64_t)(__SIZE_MAX__))
  return false;
return true;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155457#4507107 , @xgupta wrote:

> In D155457#4506405 , @xbolva00 
> wrote:
>
>> You should add a testcase which uses “expected no diagnostics”.
>
> There is some issue writing test
>
>   $ cat type-limit-compare.cpp
>   // RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare -verify 
> %s
>   // expected-no-diagnostics
>   
>   #include 
>   #include 
>   #include 
>   
>   bool foo(uint64_t Size) {
> if (sizeof(std::size_t) < sizeof(uint64_t) &&
>Size > static_cast(std::numeric_limits::max())) 
> // no-warning
>   return false;
> return true;
>   }
>
> failed with
>
>   $ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 
> 'cstddef' file not found
>   4 | #include 
> |  ^

We typically do not include any system headers (STL or otherwise) as part of 
the compiler tests; that would test whatever is found on the test system 
instead of a consistent test. Instead, I'd recommend doing:

  namespace std {
  using size_t = decltype(sizeof(0));
  }

Similarly, you can replace `uint64_t` with `unsigned long long` and the 
`numeric_limits` call with `__SIZE_MAX__`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

In D155457#4506405 , @xbolva00 wrote:

> You should add a testcase which uses “expected no diagnostics”.

There is some issue writing test

  $ cat type-limit-compare.cpp
  // RUN: %clang_cc1  -fsyntax-only -Wtautological-type-limit-compare -verify %s
  // expected-no-diagnostics
  
  #include 
  #include 
  #include 
  
  bool foo(uint64_t Size) {
if (sizeof(std::size_t) < sizeof(uint64_t) &&
   Size > static_cast(std::numeric_limits::max())) 
// no-warning
  return false;
return true;
  }

failed with

  $ llvm-project/clang/test/Sema/type-limit-compare.cpp:4:10: fatal error: 
'cstddef' file not found
  4 | #include 
|  ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

You should add a testcase which uses “expected no diagnostics”.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

I am not sure if this is correct but this passes `ninja check-clang`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155457

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

[PATCH] D155457: [clang] Skip tautological comparison if the comparison involves the 'size_t' type

[Shivam Gupta via Phabricator via cfe-commits]

xgupta created this revision.
xgupta added reviewers: aaron.ballman, xbolva00.
Herald added a project: All.
xgupta requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The issue with size_t comes when we are trying to addd
-Wtype-limits to -Wextra for GCC compatibility in review 
https://reviews.llvm.org/D142826.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155457

Files:
  clang/lib/Sema/SemaChecking.cpp


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13363,6 +13363,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13363,6 +13363,13 @@
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
 return false;
 
+  // Don't warn if the comparison involves the 'size_t' type.
+  QualType SizeT = S.Context.getSizeType();
+  if (S.Context.hasSameType(Constant->getType().getCanonicalType(), SizeT) &&
+  S.Context.hasSameType(Other->getType().getCanonicalType(), SizeT)) {
+return false;
+  }
+
   // A comparison of an unsigned bit-field against 0 is really a type problem,
   // even though at the type level the bit-field might promote to 'signed int'.
   if (Other->refersToBitField() && InRange && Value == 0 &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits