Skip to content

No longer add enumeration constants to the wrong scope #134998

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+41 −4
Open

No longer add enumeration constants to the wrong scope #134998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6864d9e
No longer add enumeration constants to the wrong scope
AaronBallman Apr 9, 2025
e76dd60
Update based on review feedback
AaronBallman Apr 9, 2025
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
5 changes: 5 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,11 @@ Bug Fixes to C++ Support
- Clang now issues an error when placement new is used to modify a const-qualified variable
in a ``constexpr`` function. (#GH131432)
- Clang now emits a warning when class template argument deduction for alias templates is used in C++17. (#GH133806)
- No longer add enumerators to the scope chain when the enumeration is declared
within a class context but is defined out of line. Previously, the
enumerators were being added both to the class context and to the namespace
scope. (#GH23317)


Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3347,6 +3347,8 @@ class EnumConstantDecl : public ValueDecl,
EnumConstantDecl *getCanonicalDecl() override { return getFirstDecl(); }
const EnumConstantDecl *getCanonicalDecl() const { return getFirstDecl(); }

bool isOutOfLine() const override;

// Implement isa/cast/dyncast/etc.
static bool classof(const Decl *D) { return classofKind(D->getKind()); }
static bool classofKind(Kind K) { return K == EnumConstant; }
Expand Down
12 changes: 12 additions & 0 deletions clang/lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5585,6 +5585,18 @@ SourceRange EnumConstantDecl::getSourceRange() const {
return SourceRange(getLocation(), End);
}

bool EnumConstantDecl::isOutOfLine() const {
if (Decl::isOutOfLine())
return true;

// In C++, if the enumeration is out of line, the enumeration constants are
// also out of line.
if (getLangOpts().CPlusPlus)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I get the point of the C++ check? I would presume that C just has no way to SPELL out-of-line enums, so this is a 'can't happen'. But I wonder if we should just depend on that and do this for each language, in case C ever figures out how to spell namespacing :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed that C has no way to spell this and from my looking, it seems like every place which calls isOutOfLine() is a C++-specific code path, but many times it seems to be accidental. For example, we have several uses that are guarded on whether something is a static data member, which is a C++-only concept, rather than guarding on C++ itself. As C integrates more C++ features, I worry that we'll end up calling this in C code as well, so that's a defensive measure. It technically can be removed, but I'm not certain we want to remove it, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can kinda see both ways here actually... there is a seemingly equal risk of "C won't standardize something enough like C++" and "C will standardize what C++ does". And the part I'd be concerned about is, "Implementer didn't realize the implications of everything, so accidentally partially implemented"

In each case we get similar bug reports, though inverted based on case. I could go either way.

return cast<Decl>(getDeclContext())->isOutOfLine();

return false;
}

void TypeDecl::anchor() {}

TypedefDecl *TypedefDecl::Create(ASTContext &C, DeclContext *DC,
Expand Down
8 changes: 4 additions & 4 deletions clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ A<int> a;
A<int>::E a0 = A<int>().v;
int n = A<int>::E::e1; // expected-error {{implicit instantiation of undefined member}}

template<typename T> enum A<T>::E : T { e1, e2 }; // expected-note 2 {{declared here}}
template<typename T> enum A<T>::E : T { e1, e2 };

// FIXME: Now that A<T>::E is defined, we are supposed to inject its enumerators
// into the already-instantiated class A<T>. This seems like a really bad idea,
// though, so we don't implement that, but what we do implement is inconsistent.
//
// Either do as the standard says, or only include enumerators lexically defined
// within the class in its scope.
A<int>::E a1 = A<int>::e1; // expected-error {{no member named 'e1' in 'A<int>'; did you mean simply 'e1'?}}
A<int>::E a1 = A<int>::e1; // expected-error {{no member named 'e1' in 'A<int>'}}

A<char>::E a2 = A<char>::e2;

Expand All @@ -43,7 +43,7 @@ B<int>::E b1 = B<int>::E::e1;

B<char>::E b2 = B<char>::E::e2;

template<typename T> typename B<T>::E B<T>::g() { return e2; }
template<typename T> typename B<T>::E B<T>::g() { return e2; } // expected-error {{use of undeclared identifier 'e2'}}
B<short>::E b3 = B<short>().g();


Expand Down Expand Up @@ -94,7 +94,7 @@ D<int>::E d1 = D<int>::E::e1; // expected-error {{incomplete type 'D<int>::E'}}
template<> enum class D<int>::E { e2 };
D<int>::E d2 = D<int>::E::e2;
D<char>::E d3 = D<char>::E::e1; // expected-note {{first required here}}
D<char>::E d4 = D<char>::E::e2; // expected-error {{no member named 'e2' in 'D<char>::E'; did you mean simply 'e2'?}}
D<char>::E d4 = D<char>::E::e2; // expected-error {{no member named 'e2' in 'D<char>::E'}}
template<> enum class D<char>::E { e3 }; // expected-error {{explicit specialization of 'E' after instantiation}}

template<> enum class D<short>::E;
Expand Down
18 changes: 18 additions & 0 deletions clang/test/SemaCXX/enum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,21 @@ class C {
// expected-error {{unexpected ';' before ')'}}
};
}

#if __cplusplus >= 201103L
namespace GH23317 {
struct A {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still going to work for:

struct A {
enum E : int;
enum E : int {ae1, ae2};

constexpr int foo() const;
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, that still compiles without diagnosing.

enum E : int;
constexpr int foo() const;
};

enum A::E : int { ae1 = 100, ae2 }; // expected-note {{'A::ae1' declared here}}

constexpr int A::foo() const { return ae1; } // This is fine
static_assert(A{}.foo() == 100, "oh no");

int foo() {
return ae1; // expected-error {{use of undeclared identifier 'ae1'; did you mean 'A::ae1'?}}
}
} // namespace GH23317
#endif // __cplusplus >= 201103L