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] Use CPlusPlus language option instead of Bool #80975

Merged

Conversation

Fznamznon
Copy link
Contributor

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.

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 llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 7, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 7, 2024

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

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.


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaChecking.cpp (+4-4)
  • (modified) clang/test/SemaOpenCL/operators.cl (+1-1)
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index b071a02ca3713f..6588aa23e1116f 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -16129,10 +16129,10 @@ static void CheckConditionalOperator(Sema &S, AbstractConditionalOperator *E,
 /// Check conversion of given expression to boolean.
 /// Input argument E is a logical expression.
 static void CheckBoolLikeConversion(Sema &S, Expr *E, SourceLocation CC) {
-  // While C23 does have bool as a keyword, we still need to run the bool-like
-  // conversion checks as bools are still not used as the return type from
-  // "boolean" operators or as the input type for conditional operators.
-  if (S.getLangOpts().Bool && !S.getLangOpts().C23)
+  // Run the bool-like conversion checks only for C since there bools are
+  // still not used as the return type from "boolean" operators or as the input
+  // type for conditional operators.
+  if (S.getLangOpts().CPlusPlus)
     return;
   if (E->IgnoreParenImpCasts()->getType()->isAtomicType())
     return;
diff --git a/clang/test/SemaOpenCL/operators.cl b/clang/test/SemaOpenCL/operators.cl
index cf359acd5acb97..76a7692a7105c8 100644
--- a/clang/test/SemaOpenCL/operators.cl
+++ b/clang/test/SemaOpenCL/operators.cl
@@ -118,6 +118,6 @@ kernel void pointer_ops(){
   bool b = !p;
   b = p==0;
   int i;
-  b = !&i;
+  b = !&i; // expected-warning {{address of 'i' will always evaluate to 'true'}}
   b = &i==(int *)1;
 }

@Fznamznon
Copy link
Contributor Author

Not sure about what to put in a release note, and whether it is necessary at all.

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.

The change in OpenCL behavior looks reasonable to me, but let's wait for @AnastasiaStulova to have a chance to weigh in.

I think we can skip a release note as this is almost an NFC change except for the OpenCL bits. We could put a release note under OpenCL for this if we wanted though.

The changes themselves LGTM barring feedback on OpenCL.

@Fznamznon
Copy link
Contributor Author

@svenvh , I wonder if you could provide some feedback for OpenCL bits here?

Copy link
Member

@svenvh svenvh left a comment

Choose a reason for hiding this comment

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

The side-effect on OpenCL looks good to me; thanks!

@Fznamznon
Copy link
Contributor Author

The side-effect on OpenCL looks good to me; thanks!

Thanks!

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!

@Fznamznon Fznamznon merged commit 8697bbe into llvm:main Feb 8, 2024
6 of 7 checks passed
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.

None yet

4 participants