Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Fix unexpected -Wconstant-logical-operand in C23 #80724

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

Fznamznon
Copy link
Contributor

C23 has bool, but logical operators still return int. Double check that we're not in C23 to avoid false-positive -Wconstant-logical-operand.

Fixes #64356

C23 has bool, but logical operators still return int. Double check that
we're not in C23 to avoid false-positive -Wconstant-logical-operand.

Fixes llvm#64356
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 5, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 5, 2024

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

C23 has bool, but logical operators still return int. Double check that we're not in C23 to avoid false-positive -Wconstant-logical-operand.

Fixes #64356


Full diff: https://github.com/llvm/llvm-project/pull/80724.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaExpr.cpp (+2-1)
  • (modified) clang/test/Sema/warn-int-in-bool-context.c (+11)
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index d15278bce5a6ba..f65b3abe5eaa24 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -14073,7 +14073,8 @@ inline QualType Sema::CheckLogicalOperands(ExprResult &LHS, ExprResult &RHS,
     Expr::EvalResult EVResult;
     if (RHS.get()->EvaluateAsInt(EVResult, Context)) {
       llvm::APSInt Result = EVResult.Val.getInt();
-      if ((getLangOpts().Bool && !RHS.get()->getType()->isBooleanType() &&
+      if ((getLangOpts().Bool && !getLangOpts().C23 &&
+           !RHS.get()->getType()->isBooleanType() &&
            !RHS.get()->getExprLoc().isMacroID()) ||
           (Result != 0 && Result != 1)) {
         Diag(Loc, diag::warn_logical_instead_of_bitwise)
diff --git a/clang/test/Sema/warn-int-in-bool-context.c b/clang/test/Sema/warn-int-in-bool-context.c
index a6890161b5af89..c111a5af23f577 100644
--- a/clang/test/Sema/warn-int-in-bool-context.c
+++ b/clang/test/Sema/warn-int-in-bool-context.c
@@ -79,3 +79,14 @@ int test(int a, unsigned b, enum num n) {
   // Don't warn in macros.
   return SHIFT(1, a);
 }
+
+int GH64356(int arg) {
+  if ((arg == 1) && (1 == 1)) return 1;
+    return 0;
+
+  if ((64 > 32) && (32 < 64))
+    return 2;
+
+  if ((1 == 1) && (arg == 1)) return 1;
+    return 0;
+}

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM though you should add a release note before landing. I think this is a good one to backport into Clang 18.x given how simple the fix is.

Thank you!

@efriedma-quic
Copy link
Collaborator

We probably shouldn't be checking for getLangOpts().Bool here at all; as documented in LangOptions.def, that flag is only supposed to indicate whether "bool" is a keyword. getLangOpts().CPlusPlus is probably appropriate.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@Fznamznon
Copy link
Contributor Author

Windows failure in CI seems unrelated.

@Fznamznon Fznamznon merged commit a18e92d into llvm:main Feb 6, 2024
4 of 5 checks passed
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 7, 2024
C23 has `bool`, but logical operators still return int. Check that
we're not in C to avoid false-positive -Wconstant-logical-operand.

Fixes llvm#64356

(cherry picked from commit a18e92d)
Fznamznon added a commit to Fznamznon/llvm-project that referenced this pull request Feb 7, 2024
As it was pointed out in llvm#80724,
we should not be checking `getLangOpts().Bool` when determining something
related to logical operators, since it only indicates that bool keyword
is present, not which semantic logical operators have.
As a side effect a missing `-Wpointer-bool-conversion` in OpenCL C was
restored since like C23, OpenCL C has bool keyword but logical operators
still return int.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 7, 2024
C23 has `bool`, but logical operators still return int. Check that
we're not in C to avoid false-positive -Wconstant-logical-operand.

Fixes llvm#64356

(cherry picked from commit a18e92d)
Fznamznon added a commit that referenced this pull request Feb 8, 2024
As it was pointed out in
#80724, we should not be
checking `getLangOpts().Bool` when determining something related to
logical operators, since it only indicates that bool keyword is present,
not which semantic logical operators have. As a side effect a missing
`-Wpointer-bool-conversion` in OpenCL C was restored since like C23,
OpenCL C has bool keyword but logical operators still return int.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 9, 2024
C23 has `bool`, but logical operators still return int. Check that
we're not in C to avoid false-positive -Wconstant-logical-operand.

Fixes llvm#64356

(cherry picked from commit a18e92d)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
C23 has `bool`, but logical operators still return int. Check that
we're not in C to avoid false-positive -Wconstant-logical-operand.

Fixes llvm#64356

(cherry picked from commit a18e92d)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
C23 has `bool`, but logical operators still return int. Check that
we're not in C to avoid false-positive -Wconstant-logical-operand.

Fixes llvm#64356

(cherry picked from commit a18e92d)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
C23 has `bool`, but logical operators still return int. Check that
we're not in C to avoid false-positive -Wconstant-logical-operand.

Fixes llvm#64356

(cherry picked from commit a18e92d)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-Wconstant-logical-operand regression with comparison operators in C2x mode with Clang 17
4 participants