=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
https://github.com/cjdb commented:
Generally speaking, I'm in favour of this; thanks so much for identifying this
problem!
When C++20 is available, we ought to suggest using `std:: is_lt` and friends
instead. Possibly something like
```
=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
AaronBallman wrote:
> Thank you for this! Overall, I like the improvements.
>
> I think the note and the fix-it are helpful. I'm more comfortable with the
> note than I am the fix-it, however, because the fix-it could result in a
> change
=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
@@ -14160,10 +14193,58 @@ static void AnalyzeComparison(Sema , BinaryOperator
*E) {
return;
}
- S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
-S.PDiag(diag::warn_mixed_sign_comparison)
-
=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
@@ -14160,10 +14193,58 @@ static void AnalyzeComparison(Sema , BinaryOperator
*E) {
return;
}
- S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
-S.PDiag(diag::warn_mixed_sign_comparison)
-
=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
@@ -14043,28 +14039,63 @@ static bool CheckTautologicalComparison(Sema ,
BinaryOperator *E,
return true;
}
+namespace {
+struct AnalyzeImplicitConversionsWorkItem {
+ Expr *E;
AaronBallman wrote:
=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
@@ -14160,10 +14193,58 @@ static void AnalyzeComparison(Sema , BinaryOperator
*E) {
return;
}
- S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
-S.PDiag(diag::warn_mixed_sign_comparison)
-
=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
https://github.com/AaronBallman commented:
Thank you for this! Overall, I like the improvements.
I think the note and the fix-it are helpful. I'm more comfortable with the note
than I am the fix-it, however, because the fix-it could result
=?utf-8?q?F=C3=A9lix?= Cloutier
Message-ID:
In-Reply-To:
@@ -7237,8 +7237,12 @@ def warn_tautological_compare_value_range : Warning<
InGroup, DefaultIgnore;
def warn_mixed_sign_comparison : Warning<
- "comparison of integers of different signs: %0 and %1">,
- InGroup,
=?utf-8?q?Félix?= Cloutier
Message-ID:
In-Reply-To:
https://github.com/AaronBallman edited
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
=?utf-8?q?F=C3=A9lix?= Cloutier
Message-ID:
In-Reply-To:
cor3ntin wrote:
@AaronBallman you have opinion on this?
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
=?utf-8?q?Félix?= Cloutier
apple-fcloutier wrote:
How do we feel about the fixit? Is it a real improvement, or is it better to
just land the warning wording improvement?
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
=?utf-8?q?Félix?= Cloutier
@@ -14308,22 +14382,22 @@ static bool AnalyzeBitFieldAssignment(Sema ,
FieldDecl *Bitfield, Expr *Init,
/// Analyze the given simple or compound assignment for warning-worthy
/// operations.
-static void AnalyzeAssignment(Sema , BinaryOperator
=?utf-8?q?Félix?= Cloutier
https://github.com/apple-fcloutier updated
https://github.com/llvm/llvm-project/pull/65684:
>From 466a68e59891e0565b8de3201305d7a881785474 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix=20Cloutier?=
Date: Wed, 6 Sep 2023 13:22:42 -0400
Subject: [PATCH 1/2]
apple-fcloutier wrote:
FWIW, this is not new behavior: https://godbolt.org/z/Mh1d43vr5
I'm less worried about unsigned -> signed promotions because they still end up
evaluating like comparisons of arbitrary-precision integers.
https://github.com/llvm/llvm-project/pull/65684
shafik wrote:
> Yes, there's this way and also when you compare integers smaller than `int`,
> but those don't trigger -Wsign-compare. -Wsign-compare only triggers if a
> signed operand is converted to an unsigned operand. This is a PR description
> bug :)
I did not understand that. I can
https://github.com/shafik review_requested
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -15478,7 +15544,7 @@ static void AnalyzeImplicitConversions(
// Ignore checking string literals that are in logical and operators.
// This is a common pattern for asserts.
continue;
-WorkList.push_back({ChildExpr, CC, IsListInit});
+
@@ -15277,35 +15351,33 @@ static void CheckImplicitConversion(Sema , Expr *E,
QualType T,
}
}
-static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
- SourceLocation CC, QualType T);
-
-static void
@@ -15423,29 +15489,29 @@ static void AnalyzeImplicitConversions(
// FIXME: Use a more uniform representation for this.
for (auto *SE : POE->semantics())
if (auto *OVE = dyn_cast(SE))
-WorkList.push_back({OVE->getSourceExpr(), CC, IsListInit});
+
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -fsyntax-only -Wsign-compare -Wno-unused-comparison
-Wno-empty-body -Wno-unused-value -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -fsyntax-only -Wsign-compare -fixit -x c %t 2> /dev/null
+// RUN: grep -v CHECK %t | FileCheck %s
+
@@ -14308,22 +14382,22 @@ static bool AnalyzeBitFieldAssignment(Sema ,
FieldDecl *Bitfield, Expr *Init,
/// Analyze the given simple or compound assignment for warning-worthy
/// operations.
-static void AnalyzeAssignment(Sema , BinaryOperator *E) {
+void
@@ -14308,22 +14382,22 @@ static bool AnalyzeBitFieldAssignment(Sema ,
FieldDecl *Bitfield, Expr *Init,
/// Analyze the given simple or compound assignment for warning-worthy
/// operations.
-static void AnalyzeAssignment(Sema , BinaryOperator *E) {
+void
@@ -15277,35 +15351,33 @@ static void CheckImplicitConversion(Sema , Expr *E,
QualType T,
}
}
-static void CheckConditionalOperator(Sema , AbstractConditionalOperator *E,
- SourceLocation CC, QualType T);
-
-static void
@@ -14135,8 +14164,8 @@ static void AnalyzeComparison(Sema , BinaryOperator
*E) {
// Go ahead and analyze implicit conversions in the operands. Note
// that we skip the implicit conversions on both sides.
- AnalyzeImplicitConversions(S, LHS, E->getOperatorLoc());
-
@@ -14308,22 +14382,22 @@ static bool AnalyzeBitFieldAssignment(Sema ,
FieldDecl *Bitfield, Expr *Init,
/// Analyze the given simple or compound assignment for warning-worthy
/// operations.
-static void AnalyzeAssignment(Sema , BinaryOperator *E) {
+void
@@ -14043,28 +14039,61 @@ static bool CheckTautologicalComparison(Sema ,
BinaryOperator *E,
return true;
}
+namespace {
+struct AnalyzeImplicitConversionsWorkItem {
+ Expr *E;
+ SourceLocation CC;
+ unsigned IsListInit : 1;
+ unsigned IsTopLevelExpr : 1;
+};
+
+class
@@ -15423,29 +15489,29 @@ static void AnalyzeImplicitConversions(
// FIXME: Use a more uniform representation for this.
for (auto *SE : POE->semantics())
if (auto *OVE = dyn_cast(SE))
-WorkList.push_back({OVE->getSourceExpr(), CC, IsListInit});
+
https://github.com/shafik commented:
Looks good overall, just the one side case I mentioned should be tested and
mostly small nits.
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
@@ -14490,12 +14564,12 @@ static void DiagnoseFloatingImpCast(Sema , Expr *E,
QualType T,
/// Analyze the given compound assignment for the possible losing of
/// floating-point precision.
-static void AnalyzeCompoundAssignment(Sema , BinaryOperator *E) {
+void
https://github.com/shafik edited https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/apple-fcloutier edited
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
apple-fcloutier wrote:
Yes, there's this way and also when you compare integers smaller than `int`,
but those don't trigger -Wsign-compare. -Wsign-compare only triggers if a
signed operand is converted to an unsigned operand. This is a PR description
bug :)
shafik wrote:
> form when the user doesn't know what happens when integers of different signs
> are compared (the signed one is cast to an unsigned value).
I just wanted to point out that although this is very much an edge case, but
mixed sign comparison can result in conversion to signed
apple-fcloutier wrote:
There's two main reasons I didn't go that route. First, we don't have great
ways to check that two expressions refer to the same object (that I'm aware
of). We can keep track of DeclRefExprs or maybe MemberExprs alone, but that
creates a weird cliff of fixit
cor3ntin wrote:
Not emitting a warning at all when there is an explicit comparison to `0` is an
interesting idea, maybe that would be worth exploring? I wonder how complicated
that would be
https://github.com/llvm/llvm-project/pull/65684
___
apple-fcloutier wrote:
IMO, the `< 0` check is fundamentally the right in-place fix to offer, although
I went back and forth on attaching it to the warning or the note. It's a little
disappointing that we still need the explicit cast to silence the warning,
however.
cor3ntin wrote:
Thanks for working on this.
I wonder if it's maybe too clever? Comparing to 0 is only one of the possible
fix, I usually try to make sure my types are consistent instead - which is not
something a fixit could do.
The more annoying thing is that this does not interact well
https://github.com/github-actions[bot] labeled
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/apple-fcloutier created
https://github.com/llvm/llvm-project/pull/65684:
Comparisons of integers with different signs are reported by -Wsign-compare
off-by-default diagnostic:
```c
int foo(int s, unsigned u) {
return s < u;
// ^ comparison of integers of
https://github.com/apple-fcloutier review_requested
https://github.com/llvm/llvm-project/pull/65684
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
40 matches
Mail list logo
