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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
