[Clang] Add '-Warray-compare' flag for C++ below version 20 #118031

AmrDeveloper · 2024-11-28T20:08:41Z

Currently, we support -wdeprecated-array-compare for C++20 or above and don't report any warning for older versions, this PR supports -Warray-compare for older versions and for GCC compatibility.

Fixes #114770

llvmbot · 2024-11-28T20:09:19Z

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

Currently, we support -wdeprecated-array-compare for C++20 or above and don't report any warning for older versions, this PR supports -Warray-compare for older versions and for GCC compatibility.

Fixes #114770

Full diff: https://github.com/llvm/llvm-project/pull/118031.diff

6 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticGroups.td (+1)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5)
(modified) clang/lib/Sema/SemaExpr.cpp (+13-4)
(modified) clang/test/SemaCXX/deprecated.cpp (+7-5)
(added) clang/test/SemaCXX/warn-array-comparion.cpp (+21)
(modified) clang/test/SemaCXX/warn-self-comparisons.cpp (+3-3)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index df9bf94b5d0398..0b57d41c617963 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -699,6 +699,7 @@ def GNUStatementExpressionFromMacroExpansion :
 def GNUStatementExpression : DiagGroup<"gnu-statement-expression",
                                        [GNUStatementExpressionFromMacroExpansion]>;
 def StringConcatation : DiagGroup<"string-concatenation">;
+def ArrayCompare : DiagGroup<"array-compare">;
 def StringCompare : DiagGroup<"string-compare">;
 def StringPlusInt : DiagGroup<"string-plus-int">;
 def StringPlusChar : DiagGroup<"string-plus-char">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 834e588c18e376..a96fddcc797d36 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -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; "
+  "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)">,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index c9d7444d5865a5..e30587459b8aff 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -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 (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
+                                       : 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.
diff --git a/clang/test/SemaCXX/deprecated.cpp b/clang/test/SemaCXX/deprecated.cpp
index 667f4d7d3edb03..03f3b2b8ff06be 100644
--- a/clang/test/SemaCXX/deprecated.cpp
+++ b/clang/test/SemaCXX/deprecated.cpp
@@ -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();
 }
 
diff --git a/clang/test/SemaCXX/warn-array-comparion.cpp b/clang/test/SemaCXX/warn-array-comparion.cpp
new file mode 100644
index 00000000000000..bda6b6969400f1
--- /dev/null
+++ b/clang/test/SemaCXX/warn-array-comparion.cpp
@@ -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}}
+}
diff --git a/clang/test/SemaCXX/warn-self-comparisons.cpp b/clang/test/SemaCXX/warn-self-comparisons.cpp
index 2e8d130bcd5a0a..daf546d5858074 100644
--- a/clang/test/SemaCXX/warn-self-comparisons.cpp
+++ b/clang/test/SemaCXX/warn-self-comparisons.cpp
@@ -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}}
 }

erichkeane

This looks good to me and does what we discussed. I'd prefer it if we could get some slightly improved diagnostic (just one that says quickly why it is problematic to compare arrays), and want someone else to take a quick peak (I've asked @shafik to do the final approval for me), else LGTM.

erichkeane · 2024-12-02T14:55:08Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

  InGroup<DeprecatedArrayCompare>;

+def warn_array_comparison : Warning<
+  "comparison between two arrays; "


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.

can you create the diagnostic group inline?

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

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.

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

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

cor3ntin

should we apply the c++26 changes at the same time?

cor3ntin · 2024-12-02T14:59:46Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

  InGroup<DeprecatedArrayCompare>;

+def warn_array_comparison : Warning<
+  "comparison between two arrays; "


can you create the diagnostic group inline?

cor3ntin · 2024-12-02T15:01:37Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

  InGroup<DeprecatedArrayCompare>;

+def warn_array_comparison : Warning<
+  "comparison between two arrays; "


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

cor3ntin · 2024-12-02T15:03:28Z

clang/lib/Sema/SemaExpr.cpp

+    auto IsDeprArrayComparion =
+        !S.getLangOpts().CPlusPlus20 || IsDeprArrayComparionIgnored;
+
+    auto DiagID = IsDeprArrayComparion ? diag::warn_array_comparison


this looks inverted

shafik · 2024-12-02T16:09:57Z

Note paper Remove Deprecated Array Comparisons from C++26 although the lastest version is not public yet.

AmrDeveloper · 2024-12-02T17:23:54Z

@shafik, @cor3ntin For C++26 there is already an issue and discussion their and i will handle it after landing this PR

#117859

shafik

This looks good but I want agreement on the diagnostic message.

shafik · 2024-12-02T17:56:39Z

clang/test/Sema/warn-stringcompare.c

    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}}


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.

CC @erichkeane

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

erichkeane

Also forgot: this needs a release note.

erichkeane · 2024-12-02T18:01:29Z

clang/include/clang/Basic/DiagnosticSemaKinds.td


+def warn_array_comparison : Warning<
+  "comparison between two arrays compare their addresses and will be deprecated in c++20; "
+  "to compare array addresses, use unary '+' to decay operands to pointers">;


Did this lose its group? It still needs one. Also, I thought someone had suggested doing it inline? Diag message seems acceptable enough to me.

I think i misunderstood inlining diagnostic here, can you give me example about how inline it

He means to inline the diagnostic group, so instead of InGroup<SomeName> you can do: InGroup<DiagnosticGroup<"Group name">>

(I think you might have to grep here for proper spelling).

Cool i found InGroup<DiagGroup<....>> thanks, for release note i think array-comparion i think it should be in New Compiler Flags section

AmrDeveloper · 2024-12-02T19:44:47Z

All comments are addressed, please take a second look.

erichkeane

Think I'm happy here with 1 nit, but please let the others take a look.

erichkeane · 2024-12-02T19:46:03Z

clang/include/clang/Basic/DiagnosticGroups.td

 def GNUStatementExpression : DiagGroup<"gnu-statement-expression",
                                       [GNUStatementExpressionFromMacroExpansion]>;
 def StringConcatation : DiagGroup<"string-concatenation">;
+def ArrayCompare : DiagGroup<"array-compare">;


Remove this please

Done, thanks

cor3ntin

Thanks for working on that, LGTM

AmrDeveloper · 2024-12-04T18:30:47Z

Should we merge now? @cor3ntin @AaronBallman @erichkeane @shafik

cor3ntin · 2024-12-04T18:31:47Z

yes. do you need me to do it?

AmrDeveloper · 2024-12-04T18:33:16Z

yes. do you need me to do it?

Thanks, i will merge now :D

shafik

Looks good, looking forward to the follow-up PR.

AmrDeveloper added 4 commits November 26, 2024 22:28

Add support for '-Warray-compare' compiler flag

9451a1e

Add lit test for array-comparison

6de4cba

Add test file to handle warn array comparion cases

fe4fab8

Update comment to add warning case in C++1a versions

bece937

AmrDeveloper requested review from AaronBallman and erichkeane November 28, 2024 20:08

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 28, 2024

AmrDeveloper added 2 commits November 28, 2024 21:36

Update Test file to expect warning in C++ mode

ef7485c

Update Cpp array compare warnings

b2034a4

erichkeane reviewed Dec 2, 2024

View reviewed changes

cor3ntin reviewed Dec 2, 2024

View reviewed changes

AmrDeveloper added 2 commits December 2, 2024 18:29

Inline diagnostic group | Remove unneeded variable

49730e1

Update Diagnostic message for array comparion

80ecba0

shafik reviewed Dec 2, 2024

View reviewed changes

erichkeane reviewed Dec 2, 2024

View reviewed changes

AmrDeveloper added 4 commits December 2, 2024 19:28

Merge branch 'llvm:main' into clang_wran_array_compare

b98e0f8

Inline diagnostic group

55e3489

Update the release note

cc67aa8

Fix releasenote indentation

d3ff02d

AmrDeveloper requested review from cor3ntin, erichkeane and shafik December 2, 2024 19:28

erichkeane reviewed Dec 2, 2024

View reviewed changes

Remove inlined Diagnostic group

3636a8f

cor3ntin approved these changes Dec 3, 2024

View reviewed changes

AmrDeveloper merged commit 7347e5e into llvm:main Dec 4, 2024
9 checks passed

AmrDeveloper mentioned this pull request Dec 5, 2024

[Clang] Implement P2865R6 (Remove Deprecated Array Comparisons from C++26) #117859

Closed

4 tasks

shafik reviewed Dec 5, 2024

View reviewed changes

[Clang] Add '-Warray-compare' flag for C++ below version 20 #118031

[Clang] Add '-Warray-compare' flag for C++ below version 20 #118031

Uh oh!

Conversation

AmrDeveloper commented Nov 28, 2024

Uh oh!

llvmbot commented Nov 28, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shafik commented Dec 2, 2024

Uh oh!

AmrDeveloper commented Dec 2, 2024

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper commented Dec 2, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper commented Dec 4, 2024

Uh oh!

cor3ntin commented Dec 4, 2024

Uh oh!

AmrDeveloper commented Dec 4, 2024

Uh oh!

Uh oh!

shafik left a comment

AmrDeveloper Dec 2, 2024 •

edited

Loading

AmrDeveloper Dec 2, 2024 •

edited

Loading