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

[xgupta]

Summary

This fix PR37919

The below code produces -Wconstant-logical-operand for the first statement,
but not the second.

void foo(int x) {
if (x && 5) {}
if (5 && x) {}
}

[xgupta]

add test cases

[xgupta]

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

[nickdesaulniers]

Thanks for the patch!

In 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.

clang/lib/Sema/SemaExpr.cpp

13596–13602

operands (plural)

13601

Please make sure to run git clang-format HEAD~ on your patches.

13611–13672

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?

[xgupta]

update test cases

[xgupta]

clang-format

[xgupta]

remov blank

[xgupta]

clang/lib/Sema/SemaExpr.cpp

13611–13672

I have tried many ways, but couldn't figure out a way to do it with function, unrelated test cases started failing.

[xbolva00]

Huge code duplication.

[xgupta]

use function to avoid code duplication

[xgupta]

remove old code

[nickdesaulniers]

clang/lib/Sema/SemaExpr.cpp

13577

Instead of LHS and RHS, should we rename these to Op1 and Op2 (short for operand one and operand two)? That way there's no confusion since this is called twice with LHS/RHS swapped the second time?

13689

Hmm...I wonder if we now care about which ExprResult contained an enum constant in bool context?

13702–13704

Probably doesn't need a duplicated comment, even with RHS/LHS switched.

[xgupta]

address comments

[xgupta]

clang/lib/Sema/SemaExpr.cpp

13689

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

[xgupta]

gentle ping for review.

[nickdesaulniers]

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.

clang/lib/Sema/SemaExpr.cpp

13578

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?

13598

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...

13598

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() ...

...

13693–13701

...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...)

[nickdesaulniers]

Hmm...looking at some of the commits to the related code, it might be very intentional that we don't warn symmetrically:

938533db602b32ab435078e723b656ac6e779a1b
e54ff6cc3e479523b71e4c7eb4bd13707d84de0f

clang/test/SemaCXX/expressions.cpp

146–148

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.

[nickdesaulniers]

clang/test/SemaCXX/expressions.cpp

146–148

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

[xgupta]

Address comments

clang/lib/Sema/SemaExpr.cpp

13598

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?"

13689

I will mark this as done, EnumConstantInBoolContext is required for warn_enum_constant_in_bool_context warning in the below code.

clang/test/SemaCXX/expressions.cpp

146–148

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?

[xbolva00]

We should not warn for macros.

if (ENABLE_XYZ && x) {}
}

This pattern is used in real world codebases.

[xgupta]

Address xbolva00's comments

[nathanchance]

I took the most recent version for a spin against the Linux kernel. The couple of issues that a previous revision of the warning flagged (https://github.com/ClangBuiltLinux/linux/issues/1806, https://github.com/ClangBuiltLinux/linux/issues/1807) are no longer visible (that seems intentional because both of those came from macro expressions) but I see a new one along a similar line as those:

https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343

In file included from drivers/gpu/drm/v3d/v3d_bo.c:25:
drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
  343 |         if (NSEC_PER_SEC % HZ &&
      |             ~~~~~~~~~~~~~~~~~ ^
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation
  343 |         if (NSEC_PER_SEC % HZ &&
      |                               ^~
      |                               &
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning
1 warning generated.

Another minimized example showing how the warning can trigger with different configurations:

$ cat x.c
#define A 1000
#define B 300
#define C 250
#define D 100

int bar(void);
int baz(void);

int foo(void)
{
        if (A % B && bar()) // 1000 % 300 = 100, warning
                return -3;

        if (A % C && bar()) // 1000 % 250 = 0, no warning
                return -2;

        if (A % D && bar()) // 1000 % 100 = 0, no warning
                return -1;

        return baz();
}

$ clang -Wall -fsyntax-only x.c
x.c:11:12: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
   11 |         if (A % B && bar())
      |             ~~~~~ ^
x.c:11:12: note: use '&' for a bitwise operation
   11 |         if (A % B && bar())
      |                   ^~
      |                   &
x.c:11:12: note: remove constant to silence this warning
1 warning generated.

I am sure we can send a patch making that a boolean expression (although it is duplicated in a few places it seems so multiple patches will be needed) but I figured I would report it just in case there was something that should be changed with the warning, since I see there was already some discussion around not warning for macros and this seems along a similar line.

[xgupta]

In D142609#4507696, @nathanchance wrote:

I took the most recent version for a spin against the Linux kernel. The couple of issues that a previous revision of the warning flagged (https://github.com/ClangBuiltLinux/linux/issues/1806, https://github.com/ClangBuiltLinux/linux/issues/1807) are no longer visible (that seems intentional because both of those came from macro expressions) but I see a new one along a similar line as those:

https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343

In file included from drivers/gpu/drm/v3d/v3d_bo.c:25:
drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
  343 |         if (NSEC_PER_SEC % HZ &&
      |             ~~~~~~~~~~~~~~~~~ ^
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation
  343 |         if (NSEC_PER_SEC % HZ &&
      |                               ^~
      |                               &
drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning
1 warning generated.

Another minimized example showing how the warning can trigger with different configurations:

$ cat x.c
#define A 1000
#define B 300
#define C 250
#define D 100

int bar(void);
int baz(void);

int foo(void)
{
        if (A % B && bar()) // 1000 % 300 = 100, warning
                return -3;

        if (A % C && bar()) // 1000 % 250 = 0, no warning
                return -2;

        if (A % D && bar()) // 1000 % 100 = 0, no warning
                return -1;

        return baz();
}

$ clang -Wall -fsyntax-only x.c
x.c:11:12: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
   11 |         if (A % B && bar())
      |             ~~~~~ ^
x.c:11:12: note: use '&' for a bitwise operation
   11 |         if (A % B && bar())
      |                   ^~
      |                   &
x.c:11:12: note: remove constant to silence this warning
1 warning generated.

I am sure we can send a patch making that a boolean expression (although it is duplicated in a few places it seems so multiple patches will be needed) but I figured I would report it just in case there was something that should be changed with the warning, since I see there was already some discussion around not warning for macros and this seems along a similar line.

WDYT @xbolva00, It is a valid warning or more modification is required in the patch?

[xbolva00]

In my opinion it makes sense to adjust that kernel code based on this warning in the current inplementation state.

@aaron.ballman ?

[aaron.ballman]

In D142609#4510596, @xbolva00 wrote:

In my opinion it makes sense to adjust that kernel code based on this warning in the current inplementation state.

@aaron.ballman ?

I think that use of macros for any of the constant values in the expression should silence the diagnostic on the assumption that the macro value changes with different configurations and so it's only constant in one configuration mode. Alternatively, I could see an argument to use a different diagnostic group when macros are involved so that users can silence macro-related constant warnings while not losing non-macro cases. WDYT?

[xgupta]

In D142609#4510995, @aaron.ballman wrote:

In D142609#4510596, @xbolva00 wrote:

In my opinion it makes sense to adjust that kernel code based on this warning in the current inplementation state.

@aaron.ballman ?

I think that use of macros for any of the constant values in the expression should silence the diagnostic on the assumption that the macro value changes with different configurations and so it's only constant in one configuration mode. Alternatively, I could see an argument to use a different diagnostic group when macros are involved so that users can silence macro-related constant warnings while not losing non-macro cases. WDYT?

IIUC in this patch I am making more like an NFC change and this could be a separate issue. WDYT @nickdesaulniers? Please review it so we can commit it soon.

[nickdesaulniers]

In D142609#4507696, @nathanchance wrote:

but I see a new one along a similar line as those:

https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343

I only see 4 instances of NSEC_PER_SEC % HZ in mainline. 2 already use the C preprocessor (inconsistently), and 2 don't. I think the 2 that don't could be converted to use the preprocessor, then that issue goes away. So I think we can clean that up on the kernel side.

@nathanchance were there many other instances of warnings triggered by this patch beyond that?

[nathanchance]

In D142609#4511579, @nickdesaulniers wrote:

In D142609#4507696, @nathanchance wrote:

but I see a new one along a similar line as those:

https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343

I only see 4 instances of NSEC_PER_SEC % HZ in mainline. 2 already use the C preprocessor (inconsistently), and 2 don't. I think the 2 that don't could be converted to use the preprocessor, then that issue goes away. So I think we can clean that up on the kernel side.

Is this diff what you had in mind?

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4a33ad2d122b..d7dd93da2511 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -186,9 +186,10 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj,
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
-           div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
+#if (NSEC_PER_SEC % HZ) != 0
+       if (div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;
+#endif

        return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
 }
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index b74b1351bfc8..af1470ee477d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -340,9 +340,10 @@ struct v3d_submit_ext {
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
-           div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
+#if (NSEC_PER_SEC % HZ) != 0
+       if (div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;
+#endif

        return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
 }

Not sure how the kernel maintainers will like that, they generally do not like to use preprocessing for hiding warnings. Alternatively, we could just turn the checks into booleans, which is what I tested earlier.

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4a33ad2d122b..d4b918fb11ce 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj,
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
+       if ((NSEC_PER_SEC % HZ) != 0 &&
            div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index b74b1351bfc8..7f664a4b2a75 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -340,7 +340,7 @@ struct v3d_submit_ext {
 static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
 {
        /* nsecs_to_jiffies64() does not guard against overflow */
-       if (NSEC_PER_SEC % HZ &&
+       if ((NSEC_PER_SEC % HZ) != 0 &&
            div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                return MAX_JIFFY_OFFSET;

I don't really have a strong preference either way but warnings that show up with certain configuration values and not others are annoying for things like randconfig testing, although I suppose that is always the nature of certain warnings...

@nathanchance were there many other instances of warnings triggered by this patch beyond that?

No, those were the only two instances that I saw across my whole build matrix with the current version of the patch.

[xgupta]

Thanks, @nickdesaulniers for reviewing and @nathanchance for testing the change.

@aaron.ballman, I also agree with @xbolva00 seems warning in kernel code is valid but I also agree with your comment about macro, may be better to track the macro-related issue with another GitHub issue.

[nathanchance]

For the record, I have sent https://lore.kernel.org/20230718-nse[e-mail]/ to address those warnings for the kernel.

[aaron.ballman]

In D142609#4513478, @xgupta wrote:

Thanks, @nickdesaulniers for reviewing and @nathanchance for testing the change.

@aaron.ballman, I also agree with @xbolva00 seems warning in kernel code is valid but I also agree with your comment about macro, may be better to track the macro-related issue with another GitHub issue.

SGTM! I've filed it in https://github.com/llvm/llvm-project/issues/63963

[aaron.ballman]

Concerns have been raised in https://github.com/llvm/llvm-project/issues/64356 that this is an undesirable change in diagnostic behavior. The diagnostic is supposed to fire when misusing a logical operand that most likely should have been a bitwise operand. There's a feeling that true && expr is plausibly done intentionally more often than true & expr.

I think we should revert the changes from this patch and in the Clang 17.x branch so that we can reevaluate the approach taken in this patch. CC @porglezomp @cjdb

[xgupta]

In D142609#4566418, @aaron.ballman wrote:

Concerns have been raised in https://github.com/llvm/llvm-project/issues/64356 that this is an undesirable change in diagnostic behavior. The diagnostic is supposed to fire when misusing a logical operand that most likely should have been a bitwise operand. There's a feeling that true && expr is plausibly done intentionally more often than true & expr.

I think we should revert the changes from this patch and in the Clang 17.x branch so that we can reevaluate the approach taken in this patch. CC @porglezomp @cjdb

This got reverted and cherry-pick request is made - https://github.com/llvm/llvm-project/issues/64515.

[aaron.ballman]

In D142609#4568095, @xgupta wrote:

In D142609#4566418, @aaron.ballman wrote:

Concerns have been raised in https://github.com/llvm/llvm-project/issues/64356 that this is an undesirable change in diagnostic behavior. The diagnostic is supposed to fire when misusing a logical operand that most likely should have been a bitwise operand. There's a feeling that true && expr is plausibly done intentionally more often than true & expr.

I think we should revert the changes from this patch and in the Clang 17.x branch so that we can reevaluate the approach taken in this patch. CC @porglezomp @cjdb

This got reverted and cherry-pick request is made - https://github.com/llvm/llvm-project/issues/64515.

Thank you! There's been some more discussion about the design on the github issue, in case you're interested in continuing to pursue this patch.

clang/lib/Sema/SemaExpr.cpp

inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS,
   if (!compType.isNull() && compType->isIntegralOrUnscopedEnumerationType())
     return compType;
   return InvalidOperands(Loc, LHS, RHS);
 }
 
 // C99 6.5.[13,14]
 inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
                                            SourceLocation Loc,
                                            BinaryOperatorKind Opc) {
   // Check vector operands differently.
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType())
     return CheckVectorLogicalOperands(LHS, RHS, Loc);
 
   bool EnumConstantInBoolContext = false;
   for (const ExprResult &HS : {LHS, RHS}) {
     if (const auto *DREHS = dyn_cast<DeclRefExpr>(HS.get())) {
       const auto *ECDHS = dyn_cast<EnumConstantDecl>(DREHS->getDecl());
       if (ECDHS && ECDHS->getInitVal() != 0 && ECDHS->getInitVal() != 1)
         EnumConstantInBoolContext = true;
     }
   }
 
   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.  We do this when the LHS is a non-bool integer and the RHS
-  // is a constant.
-  if (!EnumConstantInBoolContext && LHS.get()->getType()->isIntegerType() &&
+  // bitwise one.  We do this when one of the operand is a non-bool integer and
+  // the other is a constant.
+  if (!EnumConstantInBoolContext &&
+      LHS.get()->getType()->isIntegerType() &&
         !LHS.get()->getType()->isBooleanType() &&
-      RHS.get()->getType()->isIntegerType() && !RHS.get()->isValueDependent() &&
+        RHS.get()->getType()->isIntegerType() &&
+        !RHS.get()->isValueDependent() &&
       // Don't warn in macros or template instantiations.
       !Loc.isMacroID() && !inTemplateInstantiation()) {
-    // If the RHS can be constant folded, and if it constant folds to something
-    // that isn't 0 or 1 (which indicate a potential logical operation that
-    // happened to fold to true/false) then warn.
-    // Parens on the RHS are ignored.
+    // If the operand can be constant folded, and if it constant folds to
+    // something that isn't 0 or 1 (which indicate a potential logical operation
+    // that happened to fold to true/false) then warn. Parens on the operand are
+    // ignored.
     Expr::EvalResult EVResult;
+
     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 ? "&&" : "||");
         // Suggest replacing the logical operator with the bitwise version
         Diag(Loc, diag::note_logical_instead_of_bitwise_change_operator)
             << (Opc == BO_LAnd ? "&" : "|")
             << FixItHint::CreateReplacement(
                    SourceRange(Loc, getLocForEndOfToken(Loc)),
                    Opc == BO_LAnd ? "&" : "|");
         if (Opc == BO_LAnd)
           // Suggest replacing "Foo() && kNonZero" with "Foo()"
           Diag(Loc, diag::note_logical_instead_of_bitwise_remove_constant)
               << FixItHint::CreateRemoval(
                      SourceRange(getLocForEndOfToken(LHS.get()->getEndLoc()),
                                  RHS.get()->getEndLoc()));
       }
     }
-  }
 
+  // Diagnose cases where the user write a logical and/or but probably meant a
+  // bitwise one.  We do this when one of the operand is a non-bool integer and
+  // the other is a constant.
+  if (!EnumConstantInBoolContext &&
+      RHS.get()->getType()->isIntegerType() &&
+        !RHS.get()->getType()->isBooleanType() &&
+        LHS.get()->getType()->isIntegerType() &&
+        !LHS.get()->isValueDependent() &&
+      // Don't warn in macros or template instantiations.
+      !Loc.isMacroID() && !inTemplateInstantiation()) {
+    // If the operand can be constant folded, and if it constant folds to
+    // something that isn't 0 or 1 (which indicate a potential logical operation
+    // that happened to fold to true/false) then warn. Parens on the operand are
+    // ignored.
+    Expr::EvalResult EVResult;
+
+
+    if (LHS.get()->EvaluateAsInt(EVResult, Context)) {
+      llvm::APSInt Result = EVResult.Val.getInt();
+      if ((getLangOpts().Bool && !LHS.get()->getType()->isBooleanType() &&
+           !LHS.get()->getExprLoc().isMacroID()) ||
+          (Result != 0 && Result != 1)) {
+        Diag(Loc, diag::warn_logical_instead_of_bitwise)
+            << LHS.get()->getSourceRange() << (Opc == BO_LAnd ? "&&" : "||");
+        // Suggest replacing the logical operator with the bitwise version
+        Diag(Loc, diag::note_logical_instead_of_bitwise_change_operator)
+            << (Opc == BO_LAnd ? "&" : "|")
+            << FixItHint::CreateReplacement(
+                   SourceRange(Loc, getLocForEndOfToken(Loc)),
+                   Opc == BO_LAnd ? "&" : "|");
+        if (Opc == BO_LAnd)
+          // Suggest replacing "kNonZero && foo() " with "Foo()"
+          Diag(Loc, diag::note_logical_instead_of_bitwise_remove_constant)
+              << FixItHint::CreateRemoval(
+                     SourceRange(getLocForEndOfToken(RHS.get()->getEndLoc()),
+                                 LHS.get()->getEndLoc()));
+      }
+    }
+  }
+
   if (!Context.getLangOpts().CPlusPlus) {
     // OpenCL v1.1 s6.3.g: The logical operators and (&&), or (||) do
     // not operate on the built-in scalar and vector float types.
     if (Context.getLangOpts().OpenCL &&
         Context.getLangOpts().OpenCLVersion < 120) {
       if (LHS.get()->getType()->isFloatingType() ||
           RHS.get()->getType()->isFloatingType())
         return InvalidOperands(Loc, LHS, RHS);
     }
 
     LHS = UsualUnaryConversions(LHS.get());
     if (LHS.isInvalid())
       return QualType();
 
     RHS = UsualUnaryConversions(RHS.get());
     if (RHS.isInvalid())
       return QualType();
 
     if (!LHS.get()->getType()->isScalarType() ||
         !RHS.get()->getType()->isScalarType())
       return InvalidOperands(Loc, LHS, RHS);
 
     return Context.IntTy;
   }
 
   // The following is safe because we only use this method for
   // non-overloadable operands.
 
   // C++ [expr.log.and]p1
   // C++ [expr.log.or]p1
   // The operands are both contextually converted to type bool.
   ExprResult LHSRes = PerformContextuallyConvertToBool(LHS.get());
   if (LHSRes.isInvalid())
     return InvalidOperands(Loc, LHS, RHS);
   LHS = LHSRes;
 
   ExprResult RHSRes = PerformContextuallyConvertToBool(RHS.get());
   if (RHSRes.isInvalid())
     return InvalidOperands(Loc, LHS, RHS);

clang/test/C/drs/dr4xx.c

void dr489(void) {
 
   /* FIXME: this should be accepted in C2x and up without a diagnostic, as C23
    * added compound literals to the allowed list of things in an ICE. The
    * diagnostic is correct for C17 and earlier though.
    */
   case (int){ 2 }: break;   /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
                                c89only-warning {{compound literals are a C99-specific feature}}
                              */
-  case 12 || main(): break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}} */
+  case 12 || main(): break; /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
+                               expected-warning {{use of logical '||' with constant operand}} \
+                               expected-note {{use '|' for a bitwise operation}} */
   }
 }
 
 /* WG14 DR492: yes
  * Named Child struct-union with no Member
  */
 struct dr492_t {
   union U11 {  /* expected-warning {{declaration does not declare anything}} */

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp

 struct S { int a; int b; int c[2]; };
 constexpr S InitList(int a) { return { a, ng }; }; // expected-error {{never produces}} expected-note {{read}}
 constexpr S InitList1a(int a) { return S{ a, ng }; }; // expected-error {{never produces}} expected-note {{read}}
 constexpr S InitList2(int a) { return { a, a, { ng } }; }; // expected-error {{never produces}} expected-note {{read}}
 constexpr S InitList3(int a) { return a ? S{ a, a } : S{ a, ng }; }; // ok
 
 constexpr int LogicalAnd1(int n) { return n && (throw, 0); } // ok
 constexpr int LogicalAnd2(int n) { return 1 && (throw, 0); } // expected-error {{never produces}} expected-note {{subexpression}}
+                                                             // expected-warning@-1 {{use of logical '&&' with constant operand}}
+                                                             // expected-note@-2 {{use '&' for a bitwise operation}}
+                                                             // expected-note@-3 {{remove constant to silence this warning}}
 
 constexpr int LogicalOr1(int n) { return n || (throw, 0); } // ok
 constexpr int LogicalOr2(int n) { return 0 || (throw, 0); } // expected-error {{never produces}} expected-note {{subexpression}}
+                                                            // expected-warning@-1 {{use of logical '||' with constant operand}}
+                                                            // expected-note@-2 {{use '|' for a bitwise operation}}
+
 
 constexpr int Conditional1(bool b, int n) { return b ? n : ng; } // ok
 constexpr int Conditional2(bool b, int n) { return b ? n * ng : n + ng; } // expected-error {{never produces}} expected-note {{both arms of conditional operator are unable to produce a constant expression}}
 
 // __builtin_constant_p ? : is magical, and is always a potential constant.
 constexpr bool BcpCall(int n) {
   return __builtin_constant_p((int*)n != &n) ? (int*)n != &n : (int*)n != &n; // expected-warning 3 {{cast to 'int *' from smaller integer type 'int'}}
 }

clang/test/Parser/cxx2a-concept-declaration.cpp

 template<bool x> concept C12 = 2 && x; // expected-error {{atomic constraint must be of type 'bool' (found 'int')}}
 template<bool x> concept C13 = x || 2 || x; // expected-error {{atomic constraint must be of type 'bool' (found 'int')}}
 template<bool x> concept C14 = 8ull && x || x; // expected-error {{atomic constraint must be of type 'bool' (found 'unsigned long long')}}
 template<typename T> concept C15 = sizeof(T); // expected-error {{atomic constraint must be of type 'bool'}}
 template<typename T> concept C16 = true && (0 && 0); // expected-error {{atomic constraint must be of type 'bool' (found 'int')}}
 // expected-warning@-1{{use of logical '&&' with constant operand}}
 // expected-note@-2{{use '&' for a bitwise operation}}
 // expected-note@-3{{remove constant to silence this warning}}
+// expected-warning@-4{{use of logical '&&' with constant operand}}
+// expected-note@-5{{use '&' for a bitwise operation}}
+// expected-note@-6{{remove constant to silence this warning}}
+
+
 template<typename T> concept C17 = T{};
 static_assert(!C17<bool>);
 template<typename T> concept C18 = (bool&&)true;
 static_assert(C18<int>);
 template<typename T> concept C19 = (const bool&)true;
 static_assert(C19<int>);
 template<typename T> concept C20 = (const bool)true;
 static_assert(C20<int>);

clang/test/Sema/exprs.c

void test19(void) {
   int __attribute__((address_space(0))) *p2 = &(*(int __attribute__((address_space(0))) *)0); // Ok.
 }
 
 int test20(int x) {
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
                  // expected-note {{use '&' for a bitwise operation}} \
                  // expected-note {{remove constant to silence this warning}}
 
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+                 // expected-note {{use '&' for a bitwise operation}} \
+                 // expected-note {{remove constant to silence this warning}}
+
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   
   // no warning, this is an idiom for "true" in old C style.
   return x && (signed char)1;
 
   return x || 0;
   return x || 1;
   return x || -1; // expected-warning {{use of logical '||' with constant operand}} \
                   // expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
                  // expected-note {{use '|' for a bitwise operation}}
+  return 5 || x; // expected-warning {{use of logical '||' with constant operand}} \
+                 // expected-note {{use '|' for a bitwise operation}}
+
   return x && 0;
   return x && 1;
   return x && -1; // expected-warning {{use of logical '&&' with constant operand}} \
                   // expected-note {{use '&' for a bitwise operation}} \
                   // expected-note {{remove constant to silence this warning}}
   return x && 5; // expected-warning {{use of logical '&&' with constant operand}} \
                  // expected-note {{use '&' for a bitwise operation}} \
                  // expected-note {{remove constant to silence this warning}}

clang/test/SemaCXX/expressions.cpp

void test_enums() {
     bar(x += E_zero); // expected-error {{incompatible type}}
   }
 }
 
 int test2(int x) {
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
                    // expected-note {{use '&' for a bitwise operation}} \
                    // expected-note {{remove constant to silence this warning}}
+  return 4 && x; // expected-warning {{use of logical '&&' with constant operand}} \
+                   // expected-note {{use '&' for a bitwise operation}} \
+                   // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.
   return x && true;
   return x && false;
   return x || true;
   return x || false;
 
   return x && (unsigned)0; // expected-warning {{use of logical '&&' with constant operand}} \
                    // expected-note {{use '&' for a bitwise operation}} \
                    // expected-note {{remove constant to silence this warning}}
 
   return x || (unsigned)1; // expected-warning {{use of logical '||' with constant operand}} \
                    // expected-note {{use '|' for a bitwise operation}}
 
   return x || 0; // expected-warning {{use of logical '||' with constant operand}} \
                    // expected-note {{use '|' for a bitwise operation}}
   return x || 1; // expected-warning {{use of logical '||' with constant operand}} \
                    // expected-note {{use '|' for a bitwise operation}}
   return x || -1; // expected-warning {{use of logical '||' with constant operand}} \
                    // expected-note {{use '|' for a bitwise operation}}
   return x || 5; // expected-warning {{use of logical '||' with constant operand}} \
                    // expected-note {{use '|' for a bitwise operation}}
+  return 5 || x; // expected-warning {{use of logical '||' with constant operand}} \
+                   // expected-note {{use '|' for a bitwise operation}}
   return x && 0; // expected-warning {{use of logical '&&' with constant operand}} \
                    // expected-note {{use '&' for a bitwise operation}} \
                    // expected-note {{remove constant to silence this warning}}
   return x && 1; // expected-warning {{use of logical '&&' with constant operand}} \
                    // expected-note {{use '&' for a bitwise operation}} \
                    // expected-note {{remove constant to silence this warning}}
   return x && -1; // expected-warning {{use of logical '&&' with constant operand}} \
                    // expected-note {{use '&' for a bitwise operation}} \
namespace pr16992 {
   }
 }
 
 void test4() {
   #define X 0
   #define Y 1
   bool r1 = X || Y;
 
   #define Y2 2
   bool r2 = X || Y2; // expected-warning {{use of logical '||' with constant operand}} \
                      // expected-note {{use '|' for a bitwise operation}}
 }

clang/test/SemaCXX/warn-unsequenced.cpp

void test() {
   a = S { ++a, a++ }.n; // ok
   A { ++a, a++ }.x; // ok
   a = A { ++a, a++ }.x; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
   A { ++a, a++ }.x + A { ++a, a++ }.y; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
                                        // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
 
   (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                           // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (0 && (a = 0)) + a; // ok
+
+  (0 && (a = 0)) + a; // cxx11-warning {{use of logical '&&' with constant operand}}
+                      // cxx11-note@-1 {{use '&' for a bitwise operation}}
+                      // cxx11-note@-2 {{remove constant to silence this warning}}
+                      // cxx17-warning@-3 {{use of logical '&&' with constant operand}}
+                      // cxx17-note@-4 {{use '&' for a bitwise operation}}
+                      // cxx17-note@-5 {{remove constant to silence this warning}}
+
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                       // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+                      // cxx11-warning@-2 {{use of logical '&&' with constant operand}}
+                      // cxx11-note@-3 {{use '&' for a bitwise operation}}
+                      // cxx11-note@-4 {{remove constant to silence this warning}}
+                      // cxx17-warning@-5 {{use of logical '&&' with constant operand}}
+                      // cxx17-note@-6 {{use '&' for a bitwise operation}}
+                      // cxx17-note@-7 {{remove constant to silence this warning}}
+
 
   (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                           // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                       // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
-  (1 || (a = 0)) + a; // ok
+                      // cxx11-warning@-2 {{use of logical '||' with constant operand}}
+                      // cxx11-note@-3 {{use '|' for a bitwise operation}}
+                      // cxx17-warning@-4 {{use of logical '||' with constant operand}}
+                      // cxx17-note@-5 {{use '|' for a bitwise operation}}
+  (1 || (a = 0)) + a; // cxx11-warning {{use of logical '||' with constant operand}}
+                      // cxx11-note@-1 {{use '|' for a bitwise operation}}
+                      // cxx17-warning@-2 {{use of logical '||' with constant operand}}
+                      // cxx17-note@-3 {{use '|' for a bitwise operation}}
+
 
   (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                          // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                      // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (1 ? a : ++a) + a; // ok
   (0 ? a : a++) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                      // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}

clang/test/SemaTemplate/dependent-expr.cpp

struct B {
     {
       C c = C();
     }
   };
 }
 
 namespace PR7724 {
   template<typename OT> int myMethod()
-  { return 2 && sizeof(OT); }
+  { return 2 && sizeof(OT); } // expected-warning {{use of logical '&&' with constant operand}} \
+                              // expected-note {{use '&' for a bitwise operation}} \
+                              // expected-note {{remove constant to silence this warning}}
 }
 
 namespace test4 {
   template <typename T> T *addressof(T &v) {
     return reinterpret_cast<T*>(
              &const_cast<char&>(reinterpret_cast<const volatile char &>(v)));
   }
 }