Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,7 @@ def GNUStatementExpressionFromMacroExpansion :
def GNUStatementExpression : DiagGroup<"gnu-statement-expression",
[GNUStatementExpressionFromMacroExpansion]>;
def StringConcatation : DiagGroup<"string-concatenation">;
def ArrayCompare : DiagGroup<"array-compare">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this please

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks

def StringCompare : DiagGroup<"string-compare">;
def StringPlusInt : DiagGroup<"string-plus-int">;
def StringPlusChar : DiagGroup<"string-plus-char">;
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -10264,6 +10264,11 @@ def warn_depr_array_comparison : Warning<
"to compare array addresses, use unary '+' to decay operands to pointers">,
InGroup<DeprecatedArrayCompare>;

def warn_array_comparison : Warning<
"comparison between two arrays; "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love some slight improvement to the first part of this diagnostic. Just some level of description as to why this is a bad idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you create the diagnostic group inline?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could say "comparison between 2 arrays compare their addresses and will be deprecated in c++20"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Comparison between two arrays won't compare their contents but their addresses"? So if folks were confused as two what it did then they would know and if not then they can manually force the array-to-pointer conversion to acknowledge what they want to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking to improve the diagnostic message for both array-compare and deprecated array compare and to be the same

Copy link
Member Author

@AmrDeveloper AmrDeveloper Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "comparison between two arrays compare their addresses and will be deprecated in c++20" is good for array-compare and we keep deprecated array compare as it is

Should we go with it? or with "won't compare their contents but their addresses"?

@shafik @cor3ntin @erichkeane

"to compare array addresses, use unary '+' to decay operands to pointers">,
InGroup<ArrayCompare>;

def warn_stringcompare : Warning<
"result of comparison against %select{a string literal|@encode}0 is "
"unspecified (use an explicit string comparison function instead)">,
Expand Down
17 changes: 13 additions & 4 deletions clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11854,14 +11854,23 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
AlwaysEqual, // std::strong_ordering::equal from operator<=>
};

// C++1a [array.comp]:
// Equality and relational comparisons ([expr.eq], [expr.rel]) between two
// operands of array type.
// C++2a [depr.array.comp]:
// Equality and relational comparisons ([expr.eq], [expr.rel]) between two
// operands of array type are deprecated.
if (S.getLangOpts().CPlusPlus20 && LHSStripped->getType()->isArrayType() &&
if (S.getLangOpts().CPlusPlus && LHSStripped->getType()->isArrayType() &&
RHSStripped->getType()->isArrayType()) {
S.Diag(Loc, diag::warn_depr_array_comparison)
<< LHS->getSourceRange() << RHS->getSourceRange()
<< LHSStripped->getType() << RHSStripped->getType();
auto IsDeprArrayComparionIgnored =
S.getDiagnostics().isIgnored(diag::warn_depr_array_comparison, Loc);
auto IsDeprArrayComparion =
!S.getLangOpts().CPlusPlus20 || IsDeprArrayComparionIgnored;

auto DiagID = IsDeprArrayComparion ? diag::warn_array_comparison
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks inverted

: diag::warn_depr_array_comparison;
S.Diag(Loc, DiagID) << LHS->getSourceRange() << RHS->getSourceRange()
<< LHSStripped->getType() << RHSStripped->getType();
// Carry on to produce the tautological comparison warning, if this
// expression is potentially-evaluated, we can resolve the array to a
// non-weak declaration, and so on.
Expand Down
4 changes: 2 additions & 2 deletions clang/test/Analysis/reference.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ namespace PR13440 {
S s = { a };
S2 s2 = { a };

if (s.x != a) return;
if (s2.x != a) return;
if (s.x != a) return; // expected-warning {{comparison between two arrays}}
if (s2.x != a) return; // expected-warning {{comparison between two arrays}}

a[0] = 42;
clang_analyzer_eval(s.x[0] == 42); // expected-warning{{TRUE}}
Expand Down
10 changes: 5 additions & 5 deletions clang/test/Sema/warn-stringcompare.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=expected,cxx %s

#define DELIM "/"
#define DOT "."
Expand All @@ -15,15 +15,15 @@ void test(const char *d) {
if (NULL == "/")
return;
if ("/" != DELIM) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
return;
return; // cxx-warning@-1 {{comparison between two arrays}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to overlap w/ warn_stringcompare we probably want to do some special handling here since string literals are a special case of arrays and they have their own comparison functions.

We can do this as part of the C++26 feature implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, thats a great concern! I think you're right, but I think as well that it is ok to do in the followup

if (DELIM == "/") // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
return;
return; // cxx-warning@-1 {{comparison between two arrays}}
if (DELIM != NULL)
return;
if (NULL == DELIM)
return;
if (DOT != DELIM) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
return;
return; // cxx-warning@-1 {{comparison between two arrays}}
if (DELIM == DOT) // expected-warning {{result of comparison against a string literal is unspecified (use an explicit string comparison function instead)}}
return;
return; // cxx-warning@-1 {{comparison between two arrays}}
}
12 changes: 7 additions & 5 deletions clang/test/SemaCXX/deprecated.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,17 +246,19 @@ namespace ArithConv {

namespace ArrayComp {
int arr1[3], arr2[4];
bool b1 = arr1 == arr2; // expected-warning {{array comparison always evaluates to false}} cxx20-warning {{comparison between two arrays is deprecated}}
bool b2 = arr1 < arr2; // expected-warning {{array comparison always evaluates to a constant}} cxx20-warning {{comparison between two arrays is deprecated}}
bool b1 = arr1 == arr2; // not-cxx20-warning {{comparison between two arrays}} cxx20-warning {{comparison between two arrays is deprecated}}
// expected-warning@-1 {{array comparison always evaluates to false}}
bool b2 = arr1 < arr2; // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
// expected-warning@-1 {{array comparison always evaluates to a constant}}
__attribute__((weak)) int arr3[3];
bool b3 = arr1 == arr3; // cxx20-warning {{comparison between two arrays is deprecated}}
bool b4 = arr1 < arr3; // cxx20-warning {{comparison between two arrays is deprecated}}
bool b3 = arr1 == arr3; // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
bool b4 = arr1 < arr3; // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
#if __cplusplus > 201703L
bool b5 = arr1 <=> arr2; // cxx20-error {{invalid operands}}
#endif

int (&f())[3];
bool b6 = arr1 == f(); // cxx20-warning {{comparison between two arrays is deprecated}}
bool b6 = arr1 == f(); // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
bool b7 = arr1 == +f();
}

Expand Down
21 changes: 21 additions & 0 deletions clang/test/SemaCXX/warn-array-comparion.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s -verify=expected,not-cxx20
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -Wno-deprecated-array-compare -verify %s -verify=expected,not-cxx20
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -Wdeprecated -verify %s -verify=expected,cxx20

typedef struct {
char str[16];
int id[16];
} Object;

bool object_equal(const Object &obj1, const Object &obj2) {
if (obj1.str != obj2.str) // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
return false;
if (obj1.id != obj2.id) // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
return false;
return true;
}


void foo(int (&array1)[2], int (&array2)[2]) {
if (array1 == array2) { } // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
}
6 changes: 3 additions & 3 deletions clang/test/SemaCXX/warn-self-comparisons.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// expected-no-diagnostics
// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s -verify=expected,not-cxx20
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -Wdeprecated -verify %s -verify=expected,cxx20

void f(int (&array1)[2], int (&array2)[2]) {
if (array1 == array2) { } // no warning
if (array1 == array2) { } // not-cxx20-warning {{comparison between two arrays;}} cxx20-warning {{comparison between two arrays is deprecated}}
}