[clang] [sema] Improve -Wsign-compare (PR #65684)

[Aaron Ballman via cfe-commits]

=?utf-8?q?Félix?= Cloutier <fcloutier at apple.com>
Message-ID:
In-Reply-To: <llvm/llvm-project/pull/65684/clang at github.com>


================
@@ -14160,10 +14193,58 @@ static void AnalyzeComparison(Sema &S, BinaryOperator *E) {
       return;
   }
 
-  S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
-                        S.PDiag(diag::warn_mixed_sign_comparison)
-                            << LHS->getType() << RHS->getType()
-                            << LHS->getSourceRange() << RHS->getSourceRange());
+  // For `S < U`, propose something like `S < 0 || (typeof(U))S < U`.
+  // The whole operation needs to have no side effects, since the signed
+  // operand will be duplicated, and the unsigned operand might not execute
+  // anymore.
+  auto MixedSignDiag = S.PDiag(diag::warn_mixed_sign_comparison)
+                       << (signedOperand == RHS) << signedOperand->getType()
+                       << T << signedOperand->getSourceRange();
+  if (!E->HasSideEffects(S.Context)) {
+    bool ParenthesizeCheck =
+        !TopLevelExpr || E != TopLevelExpr->IgnoreImplicit();
+    std::string Prefix;
+    llvm::raw_string_ostream PrefixS(Prefix);
+    if (ParenthesizeCheck)
+      PrefixS << '(';
+    auto TokRange =
+        CharSourceRange::getTokenRange(signedOperand->getSourceRange());
+    bool Invalid;
+    PrefixS << Lexer::getSourceText(TokRange, S.SourceMgr, S.getLangOpts(),
+                                    &Invalid);
+    if (!Invalid) {
+      bool ParenthesizeForCast = S.getLangOpts().CPlusPlus ||
+                                 isa<BinaryOperator>(signedOperand) ||
+                                 isa<ConditionalOperator>(signedOperand);
+      bool IsSignedLHS = signedOperand == LHS;
+      bool IsLTOrLE = E->getOpcode() == BO_LT || E->getOpcode() == BO_LE;
+      bool IsGTOrGE = E->getOpcode() == BO_GT || E->getOpcode() == BO_GE;
+      if ((IsSignedLHS && IsLTOrLE) || (!IsSignedLHS && IsGTOrGE)) {
+        PrefixS << " < 0 || ";
+      } else {
+        PrefixS << " >= 0 && ";
+      }
+      if (S.getLangOpts().CPlusPlus)
+        PrefixS << "static_cast<" << T.getAsString(S.getLangOpts()) << ">";
+      else
+        PrefixS << "(" << T.getAsString(S.getLangOpts()) << ")";
+      if (ParenthesizeForCast)
+        PrefixS << "(";
+      PrefixS.flush();
----------------
AaronBallman wrote:

```suggestion
```
No need to call flush according to the API's documentation.

https://github.com/llvm/llvm-project/pull/65684