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

[Nick Desaulniers via Phabricator via cfe-commits]

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