xbolva00 created this revision.
xbolva00 added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
xbolva00 marked an inline comment as done.
xbolva00 added inline comments.


================
Comment at: clang/test/Sema/warn-stringcompare.c:4-6
+#define DELIM "/"
+#define NULL (void *)0
+
----------------
Current behaviour: https://godbolt.org/z/7nYH6N


As noted in PR, we have a poor test coverage for this warning. I think macro 
support was just overlooked. GCC warns in these cases.
Clang missed a real bug in the code I am working with, GCC caught it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70624

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/exprs.c
  clang/test/Sema/warn-stringcompare.c

Index: clang/test/Sema/warn-stringcompare.c
===================================================================
--- /dev/null
+++ clang/test/Sema/warn-stringcompare.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+#define DELIM "/"
+#define NULL (void *)0
+
+void test(const char *d) {
+  if ("/" != d) // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+  if (d == "/") // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+  if ("/" != NULL)
+    return;
+  if (NULL == "/")
+    return;
+  if ("/" != DELIM) // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+  if (DELIM == "/") // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+}
Index: clang/test/Sema/exprs.c
===================================================================
--- clang/test/Sema/exprs.c
+++ clang/test/Sema/exprs.c
@@ -119,7 +119,7 @@
 
 // PR3753
 int test12(const char *X) {
-  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified (use strncmp instead)}}
+  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified}}
 }
 
 int test12b(const char *X) {
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10338,7 +10338,6 @@
   QualType RHSType = RHS->getType();
   if (LHSType->hasFloatingRepresentation() ||
       (LHSType->isBlockPointerType() && !BinaryOperator::isEqualityOp(Opc)) ||
-      LHS->getBeginLoc().isMacroID() || RHS->getBeginLoc().isMacroID() ||
       S.inTemplateInstantiation())
     return;
 
@@ -10366,45 +10365,51 @@
     AlwaysEqual, // std::strong_ordering::equal from operator<=>
   };
 
-  if (Expr::isSameComparisonOperand(LHS, RHS)) {
-    unsigned Result;
-    switch (Opc) {
-    case BO_EQ: case BO_LE: case BO_GE:
-      Result = AlwaysTrue;
-      break;
-    case BO_NE: case BO_LT: case BO_GT:
-      Result = AlwaysFalse;
-      break;
-    case BO_Cmp:
-      Result = AlwaysEqual;
-      break;
-    default:
-      Result = AlwaysConstant;
-      break;
-    }
-    S.DiagRuntimeBehavior(Loc, nullptr,
-                          S.PDiag(diag::warn_comparison_always)
-                              << 0 /*self-comparison*/
-                              << Result);
-  } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
-    // What is it always going to evaluate to?
-    unsigned Result;
-    switch(Opc) {
-    case BO_EQ: // e.g. array1 == array2
-      Result = AlwaysFalse;
-      break;
-    case BO_NE: // e.g. array1 != array2
-      Result = AlwaysTrue;
-      break;
-    default: // e.g. array1 <= array2
-      // The best we can say is 'a constant'
-      Result = AlwaysConstant;
-      break;
+  if (!LHS->getBeginLoc().isMacroID() && !RHS->getBeginLoc().isMacroID()) {
+    if (Expr::isSameComparisonOperand(LHS, RHS)) {
+      unsigned Result;
+      switch (Opc) {
+      case BO_EQ:
+      case BO_LE:
+      case BO_GE:
+        Result = AlwaysTrue;
+        break;
+      case BO_NE:
+      case BO_LT:
+      case BO_GT:
+        Result = AlwaysFalse;
+        break;
+      case BO_Cmp:
+        Result = AlwaysEqual;
+        break;
+      default:
+        Result = AlwaysConstant;
+        break;
+      }
+      S.DiagRuntimeBehavior(Loc, nullptr,
+                            S.PDiag(diag::warn_comparison_always)
+                                << 0 /*self-comparison*/
+                                << Result);
+    } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
+      // What is it always going to evaluate to?
+      unsigned Result;
+      switch (Opc) {
+      case BO_EQ: // e.g. array1 == array2
+        Result = AlwaysFalse;
+        break;
+      case BO_NE: // e.g. array1 != array2
+        Result = AlwaysTrue;
+        break;
+      default: // e.g. array1 <= array2
+        // The best we can say is 'a constant'
+        Result = AlwaysConstant;
+        break;
+      }
+      S.DiagRuntimeBehavior(Loc, nullptr,
+                            S.PDiag(diag::warn_comparison_always)
+                                << 1 /*array comparison*/
+                                << Result);
     }
-    S.DiagRuntimeBehavior(Loc, nullptr,
-                          S.PDiag(diag::warn_comparison_always)
-                              << 1 /*array comparison*/
-                              << Result);
   }
 
   if (isa<CastExpr>(LHSStripped))
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8428,8 +8428,7 @@
 
 def warn_stringcompare : Warning<
   "result of comparison against %select{a string literal|@encode}0 is "
-  "unspecified (use strncmp instead)">,
-  InGroup<StringCompare>;
+  "unspecified">, InGroup<StringCompare>;
 
 def warn_identity_field_assign : Warning<
   "assigning %select{field|instance variable}0 to itself">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to