[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

Mon Feb 6 09:21:46 PST 2023

nickdesaulniers added inline comments.

================
Comment at: clang/lib/Sema/SemaExpr.cpp:13583
+// bitwise one.
+void Sema::diagnoseLogicalInsteadOfBitwise(ExprResult &LHS, ExprResult &RHS,
                                            SourceLocation Loc,
----------------
Instead of `LHS` and `RHS`, should we rename these to `Op1` and `Op2` (short for operand one and operand two)?  That way there's no confusion since this is called twice with LHS/RHS swapped the second time?

================
Comment at: clang/lib/Sema/SemaExpr.cpp:13635
+      if (ECDHS && ECDHS->getInitVal() != 0 && ECDHS->getInitVal() != 1)
+        EnumConstantInBoolContext = true;
+    }
----------------
Hmm...I wonder if we now care about which `ExprResult` contained an enum constant in bool context?

================
Comment at: clang/lib/Sema/SemaExpr.cpp:13648-13650
+  // Diagnose cases where the user write a logical and/or but probably meant a
+  // bitwise one.  We do this when the RHS is a non-bool integer and the LHS
+  // is a constant.
----------------
Probably doesn't need a duplicated comment, even with RHS/LHS switched.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142609