[PATCH] D70624: [Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

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

xbolva00 marked 2 inline comments as done.
xbolva00 added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8431
   "result of comparison against %select{a string literal|@encode}0 is "
-  "unspecified (use strncmp instead)">,
-  InGroup<StringCompare>;
+  "unspecified">, InGroup<StringCompare>;
 
----------------
aaron.ballman wrote:
> I think this is a regression in the diagnostic because we dropped the recommendation for how to fix the issue. Why do we need to drop that? I'd be fine if it was made less specific, like: `(use an explicit string comparison function instead)` if the concern is over `strncmp` specifically.
Yes, my concern was about strncmp. Your suggestion is fine.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:10368
 
-  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)) {
----------------
aaron.ballman wrote:
> Why do we care about the macros here? It seems just as reasonable to warn on:
> ```
> #define MACRO1 "one"
> #define MACRO2 "one
> 
> if (MACRO1 == MACRO2) // Why not warn here too?
> ```
> 
This “macro loc” check is for array comparison diagnostic. I preserved original logic of this function and enabled -Wstring-compare for macro locations.

We warn here (I will add a test).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70624