Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion clang/lib/AST/ASTStructuralEquivalence.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,29 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
else if (T1->getTypeClass() == Type::FunctionNoProto &&
T2->getTypeClass() == Type::FunctionProto)
TC = Type::FunctionNoProto;
else
else if (Context.LangOpts.C23 && !Context.StrictTypeSpelling &&
(T1->getTypeClass() == Type::Enum ||
T2->getTypeClass() == Type::Enum)) {
// In C23, if not being strict about token equivalence, we need to handle
// the case where one type is an enumeration and the other type is an
// integral type.
//
// C23 6.7.3.3p16: The enumerated type is compatible with the underlying
// type of the enumeration.
//
// Treat the enumeration as its underlying type and use the builtin type
// class comparison.
if (T1->getTypeClass() == Type::Enum) {
T1 = T1->getAs<EnumType>()->getDecl()->getIntegerType();
if (!T2->isBuiltinType() || T1.isNull()) // Sanity check
return false;
Comment on lines +887 to +888
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the null check be an assert?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if the underlying type is invalid, I’d expect us to default to int, so it should probably never be null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In general, yes, but given that I want to put this on the release branch with little time for finding fallout, I'd prefer to not assert; we mostly recover by using int as a type, but that's not a guarantee.

How about this for an idea, leave it this way, land the changes, pick the changes to 21.x, once the pick is finished, make this an assert on main?

Copy link
Member

Choose a reason for hiding this comment

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

In general, yes, but given that I want to put this on the release branch with little time for finding fallout, I'd prefer to not assert; we mostly recover by using int as a type, but that's not a guarantee.

How about this for an idea, leave it this way, land the changes, pick the changes to 21.x, once the pick is finished, make this an assert on main?

sgtm

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this for an idea, leave it this way, land the changes, pick the changes to 21.x, once the pick is finished, make this an assert on main?

Yes, please!

} else if (T2->getTypeClass() == Type::Enum) {
T2 = T2->getAs<EnumType>()->getDecl()->getIntegerType();
if (!T1->isBuiltinType() || T2.isNull()) // Sanity check
return false;
}
TC = Type::Builtin;
} else
return false;
}

Expand Down
36 changes: 36 additions & 0 deletions clang/test/C/C23/n3037.c
Original file line number Diff line number Diff line change
Expand Up @@ -401,3 +401,39 @@ _Static_assert(0 == _Generic(inner_anon_tagged.untagged, struct { int i; } : 1,
// unions and structures are both RecordDecl objects, whereas EnumDecl is not).
enum { E_Untagged1 } nontag_enum; // both-note {{previous definition is here}}
_Static_assert(0 == _Generic(nontag_enum, enum { E_Untagged1 } : 1, default : 0)); // both-error {{redefinition of enumerator 'E_Untagged1'}}

// Test that enumerations are compatible with their underlying type, but still
// diagnose when "same type" is required rather than merely "compatible type".
enum E1 : int { e1 }; // Fixed underlying type
enum E2 { e2 }; // Unfixed underlying type, defaults to int or unsigned int

struct GH149965_1 { int h; };
// This typeof trick is used to get the underlying type of the enumeration in a
// platform agnostic way.
struct GH149965_2 { __typeof__(+(enum E2){}) h; };
void gh149965(void) {
extern struct GH149965_1 x1; // c17-note {{previous declaration is here}}
extern struct GH149965_2 x2; // c17-note {{previous declaration is here}}

// Both the structure and the variable declarations are fine because only a
// compatible type is required, not the same type, because the structures are
// declared in different scopes.
struct GH149965_1 { enum E1 h; };
struct GH149965_2 { enum E2 h; };

extern struct GH149965_1 x1; // c17-error {{redeclaration of 'x1'}}
extern struct GH149965_2 x2; // c17-error {{redeclaration of 'x2'}}

// However, in the same scope, the same type is required, not just compatible
// types.
// FIXME: this should be an error in both C17 and C23 mode.
struct GH149965_3 { int h; }; // c17-note {{previous definition is here}}
struct GH149965_3 { enum E1 h; }; // c17-error {{redefinition of 'GH149965_3'}}

// For Clang, the composite type after declaration merging is the enumeration
// type rather than an integer type.
enum E1 *eptr;
[[maybe_unused]] __typeof__(x1.h) *ptr = eptr;
enum E2 *eptr2;
[[maybe_unused]] __typeof__(x2.h) *ptr2 = eptr2;
}