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

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13635
+      if (ECDHS && ECDHS->getInitVal() != 0 && ECDHS->getInitVal() != 1)
+        EnumConstantInBoolContext = true;
+    }
----------------
xgupta wrote:
> nickdesaulniers wrote:
> > Hmm...I wonder if we now care about which `ExprResult` contained an enum constant in bool context?
> IIUC you mean EnumConstantInBoolContext is not required, but then test cases start failing if I will not use it in diagnoseLogicalInsteadOfBitwise above.
> Like this - 
> 
> $ build/bin/llvm-lit -v clang/test/Sema
> 
> Command Output (stderr):
> --
> error: 'warning' diagnostics seen but not expected: 
>   File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 52: use of logical '||' with constant operand
>   File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 56: use of logical '||' with constant operand
>   File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 68: use of logical '&&' with constant operand
> error: 'note' diagnostics seen but not expected: 
>   File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 52: use '|' for a bitwise operation
>   File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 56: use '|' for a bitwise operation
>   File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 68: use '&' for a bitwise operation
>   File /home/xgupta/compiler/llvm-project/clang/test/Sema/warn-int-in-bool-context.c Line 68: remove constant to silence this warning
> 7 errors generated.
> 
> --
> Failed Tests (1):
>   Clang :: Sema/warn-int-in-bool-context.c
> 
> 
> Testing Time: 0.04s
>   Failed: 1
> 
I will mark this as done, EnumConstantInBoolContext is required for warn_enum_constant_in_bool_context warning in the below code.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13588
+                                           bool EnumConstantInBoolContext) {
+  if (!EnumConstantInBoolContext && Op1.get()->getType()->isIntegerType() &&
+      !Op1.get()->getType()->isBooleanType() &&
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > This is the only use of `EnumConstantInBoolContext` in `Sema::diagnoseLogicalInsteadOfBitwise`. It's being used to elide the entirety of the method.  In that case, it seems wasteful to bother to pass it as a parameter.  Instead, I don't think `Sema::diagnoseLogicalInsteadOfBitwise` should be called if `EnumConstantInBoolContext` is `true`.
> > 
> > If you remove the parameter `EnumConstantInBoolContext` then...
> Do you mind pulling the types into dedicated variables, then reusing them? I kind of hate seeing verbose OpX.get()->getType() so much in this method.
> 
> Type T1 = Op1.get()->getType();
> Type T2 = Op2.get()->getType();
> 
> if (T1->isIntegerType() && !T1->isBooleanType() ...
>   ...
Done by the first comment- 
"Every reference to Op1 and Op2 immediately calls .get() on it. That's annoying. How about Sema::diagnoseLogicalInsteadOfBitwise accepts Expr* (or whatever ExprResult::get returns), and we call .get() in the caller?"




================
Comment at: clang/test/SemaCXX/expressions.cpp:146-148
   #define Y2 2
   bool r2 = X || Y2; // expected-warning {{use of logical '||' with constant operand}} \
                      // expected-note {{use '|' for a bitwise operation}}
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > So I think we'll want to change this test.
> > 
> > See commit d6eb2b9f4d4fc236376e3a5a7b8faa31e8dd427d that introduced it.
> > 
> > If we have a constant that was defined via macro, we DONT want to warn for it.
> Another related issue is that sometimes we set these constants via `-D` flags. I wonder if that's a clang bug that those aren't considered as having a valid macro id?
> 
> See also https://github.com/ClangBuiltLinux/linux/issues/1806
> So I think we'll want to change this test.
But it is not failing, what changes you want there?

> I wonder if that's a clang bug that those aren't considered as having a valid macro id?
 Will it require to address in this patch?


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

https://reviews.llvm.org/D142609