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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Thanks for the patch!

In D142609#4082259 <https://reviews.llvm.org/D142609#4082259>, @xgupta wrote:

> WIP && TODO- Fixed test
>
> Failed Tests (7):
>
>   Clang :: C/drs/dr4xx.c
>   Clang :: Modules/explicit-build-extra-files.cpp
>   Clang :: Parser/cxx2a-concept-declaration.cpp
>   Clang :: SemaCXX/expressions.cpp
>   Clang :: SemaCXX/warn-unsequenced.cpp
>   Clang :: SemaObjC/unguarded-availability.m
>   Clang :: SemaTemplate/dependent-expr.cpp

If the current diff is causing test failures, please mark this "Changes Planned" in phabricator (Under the Add Action dropdown in the bottom left) so the reviewers know that there's outstanding issues. Or post but don't cc explicit reviewers yet.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:13596
   // Diagnose cases where the user write a logical and/or but probably meant a
-  // bitwise one.  We do this when the LHS is a non-bool integer and the RHS
-  // is a constant.
-  if (!EnumConstantInBoolContext && LHS.get()->getType()->isIntegerType() &&
-      !LHS.get()->getType()->isBooleanType() &&
-      RHS.get()->getType()->isIntegerType() && !RHS.get()->isValueDependent() &&
+  // bitwise one.  We do this when one of the operand is a non-bool integer and
+  // the other is a constant.
----------------
operands (plural)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13601
+      LHS.get()->getType()->isIntegerType() && !LHS.get()->isValueDependent()) ||
+      ( RHS.get()->getType()->isIntegerType() && !RHS.get()->getType()->isBooleanType() &&
+      LHS.get()->getType()->isIntegerType() && !LHS.get()->isValueDependent())) &&
----------------
Please make sure to run `git clang-format HEAD~` on your patches.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13611-13653
     if (RHS.get()->EvaluateAsInt(EVResult, Context)) {
       llvm::APSInt Result = EVResult.Val.getInt();
       if ((getLangOpts().Bool && !RHS.get()->getType()->isBooleanType() &&
            !RHS.get()->getExprLoc().isMacroID()) ||
           (Result != 0 && Result != 1)) {
         Diag(Loc, diag::warn_logical_instead_of_bitwise)
             << RHS.get()->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");
----------------
There seems to be a lot of duplication between the two; should we move these into a static function that's called twice? Perhaps Op1 and Op2 would be better identifiers than LHS RHS in that case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142609