nickdesaulniers added a comment.
I did kernel builds of x86_64 and aarch64 defconfigs. This found new instance:
https://github.com/ClangBuiltLinux/linux/issues/1806
which looks like something we can fix in the kernel sources. Our CI will
probably find more instances once this lands, but I'm happy with it.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:13584
+// the other is a constant.
+void Sema::diagnoseLogicalInsteadOfBitwise(ExprResult &Op1, ExprResult &Op2,
SourceLocation Loc,
----------------
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/lib/Sema/SemaExpr.cpp:13588
+ bool EnumConstantInBoolContext) {
+ if (!EnumConstantInBoolContext && Op1.get()->getType()->isIntegerType() &&
+ !Op1.get()->getType()->isBooleanType() &&
----------------
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...
================
Comment at: clang/lib/Sema/SemaExpr.cpp:13588
+ bool EnumConstantInBoolContext) {
+ if (!EnumConstantInBoolContext && Op1.get()->getType()->isIntegerType() &&
+ !Op1.get()->getType()->isBooleanType() &&
----------------
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() ...
...
================
Comment at: clang/lib/Sema/SemaExpr.cpp:13640-13648
+ if (EnumConstantInBoolContext)
+ Diag(Loc, diag::warn_enum_constant_in_bool_context);
+
+ // Diagnose cases where the user write a logical and/or but probably meant a
+ // bitwise one.
+ diagnoseLogicalInsteadOfBitwise(LHS, RHS, Loc, Opc,
+ EnumConstantInBoolContext);
----------------
...this can become:
```
if (EnumConstantInBoolContext) {
Diag(...
} else {
diagnoseLogicalInsteadOfBitwise(...
diagnoseLogicalInsteadOfBitwise(...
}
```
```
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index e61b9252901d..47cfd0884911 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -12569,8 +12569,7 @@ public:
BinaryOperatorKind Opc);
void diagnoseLogicalInsteadOfBitwise(ExprResult &Op1, ExprResult &Op2,
SourceLocation Loc,
- BinaryOperatorKind Opc,
- bool EnumConstantInBoolContext);
+ BinaryOperatorKind Opc);
QualType CheckLogicalOperands( // C99 6.5.[13,14]
ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
BinaryOperatorKind Opc);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 7acf6f41625e..fa64b0cdbe94 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -13583,9 +13583,8 @@ inline QualType Sema::CheckBitwiseOperands(ExprResult
&LHS, ExprResult &RHS,
// the other is a constant.
void Sema::diagnoseLogicalInsteadOfBitwise(ExprResult &Op1, ExprResult &Op2,
SourceLocation Loc,
- BinaryOperatorKind Opc,
- bool EnumConstantInBoolContext) {
- if (!EnumConstantInBoolContext && Op1.get()->getType()->isIntegerType() &&
+ BinaryOperatorKind Opc) {
+ if (Op1.get()->getType()->isIntegerType() &&
!Op1.get()->getType()->isBooleanType() &&
Op2.get()->getType()->isIntegerType() && !Op2.get()->isValueDependent()
&&
// Don't warn in macros or template instantiations.
@@ -13639,13 +13638,12 @@ inline QualType Sema::CheckLogicalOperands(ExprResult
&LHS, ExprResult &RHS,
if (EnumConstantInBoolContext)
Diag(Loc, diag::warn_enum_constant_in_bool_context);
-
- // Diagnose cases where the user write a logical and/or but probably meant a
- // bitwise one.
- diagnoseLogicalInsteadOfBitwise(LHS, RHS, Loc, Opc,
- EnumConstantInBoolContext);
- diagnoseLogicalInsteadOfBitwise(RHS, LHS, Loc, Opc,
- EnumConstantInBoolContext);
+ else {
+ // Diagnose cases where the user write a logical and/or but probably meant
+ // a bitwise one.
+ diagnoseLogicalInsteadOfBitwise(LHS, RHS, Loc, Opc);
+ diagnoseLogicalInsteadOfBitwise(RHS, LHS, Loc, Opc);
+ }
if (!Context.getLangOpts().CPlusPlus) {
// OpenCL v1.1 s6.3.g: The logical operators and (&&), or (||) do
```
(That diff probably needs to be reformatted...)
Repository:
rG LLVM Github Monorepo
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
