-
Notifications
You must be signed in to change notification settings - Fork 419
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
dyno: implement opaque extern types #22707
Conversation
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.
Generally looks good, but I think we need to address the linkage-name vs. original-name question.
frontend/lib/resolution/Resolver.cpp
Outdated
if (typeExpr == nullptr && initExpr == nullptr && | ||
var->linkage() == uast::Decl::EXTERN && | ||
var->storageKind() == QualifiedType::TYPE) { | ||
debuggerBreakHere(); |
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.
I think this is left over from development.
|
||
class ExternType final : public Type { | ||
private: | ||
UniqueString linkageName_; |
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.
I believe we need to preserve both the linkage name and the Chapel name. If a user writes:
extern "helper_t" type helper;
writeln(helper:string);
Then I believe it should print "helper" (and does in production, fwiw).
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.
Adding a test that checks the stringify'd name of extern types would also be useful.
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.
Discussed this out of band, and Ben and I agreed that we can merge the PR as-is for the time being, and figure out how to keep the original name as part of the type later.
We don't want to show linkage names to the user instead of the Chapel type name, so this feature is important in the long term.
The reason this doesn't immediately work is that our type comparisons in canPass
are based on pointer equality; however, if we were to store the ID or name of the variable that declared an opaque type in addition to the linkage name, two variables with distinct Chapel names but identical linkage names would not be the exact same type, and thus become incompatible. We don't want that. To address the issue, we probably want to do a deep comparison on types.
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.
This is also tracked here: #22711
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
This PR implements opaque extern types in Dyno. An opaque extern type is one defined as follows:
Types like these are "opaque" in the sense that Chapel doesn't know their underlying representation; the name is pretty much the only identifying information about such types.
This PR diverges from production compiler behavior in that extern types with the same name are considered equal, even if they were declared in different places. In other words,
Reviewed by @benharsh -- thanks!
Testing