[clang] Support `__is_trivially_copyable(int()&)==false` #81298

AMP999 · 2024-02-09T18:40:11Z

IMHO it would be productive to make a similar change for typeid, in ParseCXXTypeid, in order to improve Clang's error message for https://godbolt.org/z/oKKWxeYra
But that might be better done by adding a new DeclaratorContext specifically for TypeidArg, instead of pretending that the argument to typeid is a template argument, because I don't know what else that change might affect.

Fixes #77585

IMHO it would be productive to make a similar change for `typeid`, in `ParseCXXTypeid`, in order to improve Clang's error message for https://godbolt.org/z/oKKWxeYra But that might be better done by adding a new DeclaratorContext specifically for TypeidArg, instead of pretending that the argument to `typeid` is a template argument, because I don't know what else that change might affect. Fixes llvm#77585

llvmbot · 2024-02-09T18:40:44Z

@llvm/pr-subscribers-clang

Author: Amirreza Ashouri (AMP999)

Changes

IMHO it would be productive to make a similar change for typeid, in ParseCXXTypeid, in order to improve Clang's error message for https://godbolt.org/z/oKKWxeYra
But that might be better done by adding a new DeclaratorContext specifically for TypeidArg, instead of pretending that the argument to typeid is a template argument, because I don't know what else that change might affect.

Fixes #77585

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

1 Files Affected:

(modified) clang/lib/Parse/ParseExprCXX.cpp (+4-2)

diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp
index fd262ff31e661a..746a8b2286ac4c 100644
--- a/clang/lib/Parse/ParseExprCXX.cpp
+++ b/clang/lib/Parse/ParseExprCXX.cpp
@@ -3899,7 +3899,9 @@ ExprResult Parser::ParseTypeTrait() {
   SmallVector<ParsedType, 2> Args;
   do {
     // Parse the next type.
-    TypeResult Ty = ParseTypeName();
+    TypeResult Ty = ParseTypeName(nullptr, getLangOpts().CPlusPlus
+                                               ? DeclaratorContext::TemplateArg
+                                               : DeclaratorContext::TypeName);
     if (Ty.isInvalid()) {
       Parens.skipToEnd();
       return ExprError();
@@ -3941,7 +3943,7 @@ ExprResult Parser::ParseArrayTypeTrait() {
   if (T.expectAndConsume())
     return ExprError();
 
-  TypeResult Ty = ParseTypeName();
+  TypeResult Ty = ParseTypeName(nullptr, DeclaratorContext::TemplateArg);
   if (Ty.isInvalid()) {
     SkipUntil(tok::comma, StopAtSemi);
     SkipUntil(tok::r_paren, StopAtSemi);

erichkeane

This would need a release note + I think it needs better testing. @AaronBallman might wish to take a look as well, this is his code ownership.

erichkeane · 2024-02-12T15:00:26Z

clang/test/Sema/static-assert.c

@@ -57,7 +57,7 @@ UNION(char[2], short) u2 = { .one = { 'a', 'b' } }; // ext-warning 3 {{'_Static_
 typedef UNION(char, short) U3; // expected-error {{static assertion failed due to requirement 'sizeof(char) == sizeof(short)': type size mismatch}} \
                               // expected-note{{evaluates to '1 == 2'}} \
                               // ext-warning 3 {{'_Static_assert' is a C11 extension}}
-typedef UNION(float, 0.5f) U4; // expected-error {{expected a type}} \
+typedef UNION(float, 0.5f) U4; // expected-error-re {{{{type name requires a specifier or qualifier|expected a type}}}} \


This test doesn't seem right... why is there an 'or' here?

I think there's an 'or' because the RUN lines run this in both C and C++ mode, so you get different diagnostics. A better way to express that is with a different -verify prefix on the RUN lines so that you can do something like cpp-error {{blah}} and c-error {{blah}} to make it more clear what's happening.

cor3ntin · 2024-02-22T09:33:17Z

clang/lib/Parse/ParseExprCXX.cpp

@@ -3899,7 +3899,9 @@ ExprResult Parser::ParseTypeTrait() {
  SmallVector<ParsedType, 2> Args;
  do {
    // Parse the next type.
-    TypeResult Ty = ParseTypeName();
+    TypeResult Ty = ParseTypeName(nullptr, getLangOpts().CPlusPlus


Suggested change

TypeResult Ty = ParseTypeName(nullptr, getLangOpts().CPlusPlus

TypeResult Ty = ParseTypeName(/*SourceRange=*/nullptr, getLangOpts().CPlusPlus

cor3ntin

This is going to need additional tests (ie a test for __is_trivially_copyable(int()&) would be a good start!)

cor3ntin · 2024-02-22T09:33:27Z

clang/lib/Parse/ParseExprCXX.cpp

@@ -3941,7 +3943,7 @@ ExprResult Parser::ParseArrayTypeTrait() {
  if (T.expectAndConsume())
    return ExprError();

-  TypeResult Ty = ParseTypeName();
+  TypeResult Ty = ParseTypeName(nullptr, DeclaratorContext::TemplateArg);


Suggested change

TypeResult Ty = ParseTypeName(nullptr, DeclaratorContext::TemplateArg);

TypeResult Ty = ParseTypeName(/*SourceRange=*/nullptr, DeclaratorContext::TemplateArg);

…t.cpp`

AMP999 · 2024-03-04T20:22:19Z

@cor3ntin I've reflected your suggestions in the code. Could you help me to land this PR, if it looks good to you?

cor3ntin

LGTM

…for abominable types With the llvm#81298 being approved, as soon as it gets merged into the `main` branch, we can completely remove the `has_unique_object_representations` struct and use the builtin instead.

fahadnayyar · 2024-03-27T20:30:35Z

Hey folks,

Because of this change the error message for certain type traits changed.
For example:

void f(void) {
  __has_nothrow_copy();
}

Earlier the error message was: expected a type
New error message is: type name requires a specifier or qualifier

Ref: https://godbolt.org/z/d4884bYz3

I personally feel the previous error made more sense for this case as the type name is missing in the type trait call. If the type name is present but the qualifier or specifier are not present then only the new message makes sense.

What do you think, is it a regression or correct behaviour?
@AMP999 @cor3ntin @AaronBallman

AaronBallman · 2024-03-28T14:36:22Z

I think the former diagnostic is more clear. It seems the issue is around here:

llvm-project/clang/lib/Parse/ParseDecl.cpp

Line 2828 in d7975c9

if (isTypeSpecifier(DSC) && !DS.hasTypeSpecifier()) {

We changed the DeclSpecContext so we hit the else if rather than the if because of isTypeSpecifier().

fahadnayyar · 2024-03-28T19:30:29Z

@AaronBallman precisely!
DSC now is : DSC_template_arg
DSC in earlier clang was : DSC_type_specifier

@AMP999 do you have thoughts about how we can fix this issue?

fahadnayyar · 2024-03-28T19:43:30Z

Filed this issue: #86997

AMP999 · 2024-03-29T07:41:30Z

I don't know, but it seems to me that ideally the DSC here would be DSC_template_type_arg, not just DSC_template_arg. Is there a way to make that happen?

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 9, 2024

shafik requested a review from erichkeane February 10, 2024 03:20

[clang] Fix the failing test in llvm#81298

4c58f4f

erichkeane reviewed Feb 12, 2024

View reviewed changes

AMP999 added 4 commits February 12, 2024 23:47

Add a release note.

03c2cfb

Remove trailing white space in ReleaseNotes.rst.

0de0aa0

Make C mode and C++ mode tests separated.

8c92b1e

Merge branch 'main' into pr8-fix-issue-77585

7f98022

cor3ntin reviewed Feb 22, 2024

View reviewed changes

AMP999 added 4 commits February 26, 2024 16:36

Merge branch 'main' into pr8-fix-issue-77585

1961dba

[Clang]Add new test cases to `clang/test/SemaCXX/type-traits-nonobjec…

eb4aaad

…t.cpp`

Update type-traits-nonobject.cpp

28258fc

Revise the style of code in ParseExprCXX.cpp

832b817

cor3ntin approved these changes Mar 4, 2024

View reviewed changes

cor3ntin merged commit 690bf64 into llvm:main Mar 5, 2024
5 checks passed

AMP999 mentioned this pull request Mar 5, 2024

[clang][NFC] Refactor clang/test/SemaCXX/type-traits.cpp to use modern static_assert #77584

Merged

AMP999 deleted the pr8-fix-issue-77585 branch March 16, 2024 22:19

fahadnayyar mentioned this pull request Mar 28, 2024

Error message got changed for certain type traits due to 690bf64f077a #86997

Closed

MitalAshok mentioned this pull request Jul 26, 2024

[clang][NFC] Add CWG713 test; add example from CWG1584 test #100747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Support `__is_trivially_copyable(int()&)==false` #81298

[clang] Support `__is_trivially_copyable(int()&)==false` #81298

AMP999 commented Feb 9, 2024

llvmbot commented Feb 9, 2024

erichkeane left a comment

erichkeane Feb 12, 2024

AaronBallman Feb 12, 2024

cor3ntin Feb 22, 2024

cor3ntin left a comment

cor3ntin Feb 22, 2024

AMP999 commented Mar 4, 2024 •

edited

Loading

cor3ntin left a comment

fahadnayyar commented Mar 27, 2024

AaronBallman commented Mar 28, 2024

fahadnayyar commented Mar 28, 2024

fahadnayyar commented Mar 28, 2024

AMP999 commented Mar 29, 2024

	TypeResult Ty = ParseTypeName(nullptr, getLangOpts().CPlusPlus
	TypeResult Ty = ParseTypeName(/SourceRange=/nullptr, getLangOpts().CPlusPlus

	TypeResult Ty = ParseTypeName(nullptr, DeclaratorContext::TemplateArg);
	TypeResult Ty = ParseTypeName(/SourceRange=/nullptr, DeclaratorContext::TemplateArg);

[clang] Support __is_trivially_copyable(int()&)==false #81298

[clang] Support __is_trivially_copyable(int()&)==false #81298

Conversation

AMP999 commented Feb 9, 2024

llvmbot commented Feb 9, 2024

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Feb 12, 2024

Choose a reason for hiding this comment

AaronBallman Feb 12, 2024

Choose a reason for hiding this comment

cor3ntin Feb 22, 2024

Choose a reason for hiding this comment

cor3ntin left a comment

Choose a reason for hiding this comment

cor3ntin Feb 22, 2024

Choose a reason for hiding this comment

AMP999 commented Mar 4, 2024 • edited Loading

cor3ntin left a comment

Choose a reason for hiding this comment

fahadnayyar commented Mar 27, 2024

AaronBallman commented Mar 28, 2024

fahadnayyar commented Mar 28, 2024

fahadnayyar commented Mar 28, 2024

AMP999 commented Mar 29, 2024

[clang] Support `__is_trivially_copyable(int()&)==false` #81298

[clang] Support `__is_trivially_copyable(int()&)==false` #81298

AMP999 commented Mar 4, 2024 •

edited

Loading