================
@@ -1160,8 +1160,11 @@ class CFGBuilder {
return {};
// Check that it is the same variable on both sides.
- if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
- return {};
+ if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2)) {
+ if (!Expr::isSameComparisonOperand(DeclExpr1->IgnoreParenImpCasts(),
+ DeclExpr2->IgnoreParenImpCasts()))
+ return {};
----------------
codemzs wrote:
I call `isSameComparisonOperand` **twice** (with and without striping away
explicit handles to implicit casts to preserve existing behavior), but let me
explain why the extra `IgnoreParenImpCasts()` call is necessary:
`isSameComparisonOperand()` only recurses past an `ImplicitCastExpr` when the
two cast nodes have the same CastKind (see the early‑exit check in the existing
implementation).
In the motivating bug, the two operands differ in both the number and the kind
of implicit casts:
```code
LHS: ImplicitCastExpr <LValueToRValue>
DeclRefExpr ‘__fp16 &’ (lvalue)
RHS: ImplicitCastExpr <FloatingCast>
ImplicitCastExpr <LValueToRValue>
DeclRefExpr ‘__fp16 &’ (lvalue)
```
Because `FloatingCast` != `LValueToRValue` the first comparison fails on the
CastKind test and the function bails out before it ever reaches the underlying
`DeclRefExprs`.
Calling `IgnoreParenImpCasts()` on the first failure path in
`CheckIncorrectLogicOperator`:
1. **Preserves the original fast‑path** (identical cast stacks are still
handled without any extra work).
2. Avoids touching `isSameComparisonOperand()`, whose current behavior is
relied upon by other clients in Sema and Analysis. Broadening it to ignore all
cast‑kind differences would risk new false‑positives in those callers.
3. Targets only the problematic pattern: same variable, operands differ solely
by implicit casts or parens.
Empirically, with this fallback in place we now diagnose the half‑precision
tautology.
The local fallback in `CheckIncorrectLogicOperator` keeps the original fast
path intact, touches only this diagnostic, and fixes the FP16 false‑negative
(and the crash) without affecting other warnings.
Happy to add a code comment to spell this out if that helps. Let me know
whether that addresses the concern, or if you’d still prefer a different
approach.
https://github.com/llvm/llvm-project/pull/149972
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
