-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add ExternDecl and ExternType for extern classes. #3893
Conversation
Depends on #3891 |
8c6885a
to
5ea115e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One potential concern, but if it is a real problem, it can be addressed in a separate PR.
case CARBON_KIND(SemIR::ClassType class_type): { | ||
prev_class_id = class_type.class_id; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this case happen? I'm cautious here because I don't think we want any previous name that names a class type to be merged. Eg, if the previous declaration was an ImportRef
naming alias C = OtherClass;
, then that shouldn't merge with class C
even though it was a class type. But if I'm reading ResolvePrevInstForMerge
correctly, you'll get a ClassType
in both the case of an imported class declaration and in the case of any other imported declaration that constant-evaluates to a class type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically I agree, but I'd say what you're mentioning (with alias) is not supported right now.
Bear in mind an ImportRef only has the constant value to point at the instruction in the current IR. This is all we have to work with, without a major refactoring. Which maybe should occur, and I have already have spots I'm thinking about this, but that's very much out-of-scope.
This expands `extern` handling for most cases.
This expands `extern` handling for most cases.
This expands
extern
handling for most cases.