-
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
Accept generic parameter lists in class declarations. #3933
Accept generic parameter lists in class declarations. #3933
Conversation
Check that they match across redeclarations.
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. Feel free to merge.
class Generic(T:! type) { | ||
} | ||
|
||
// --- fail_invalid.carbon |
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.
FWIW, you might consider splitting each of these to separate files, so that each is tested to independently fail. i.e., right now if one stops failing and we miss it in review, the others failing will lead to this test passing.
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.
Done.
if (EntityHasParamError(context, new_entity) || | ||
EntityHasParamError(context, prev_entity)) { | ||
return false; | ||
} |
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.
Is it worth trying to treat as a match if both have errors, and in the same position?
Alternately, would it be worth trying to only check for an error after a mismatch is found, right before emitting? i.e., so that the cost of error checking only happens on the error path. This is fetching the underlying param structs which may remain in cache, but even then it feels a bit like extra logic to we might be able to avoid.
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.
Might be interesting to pursue that, yeah. For now this is just moved from function merging without changing the logic, so I'm not going to handle this in this PR.
toolchain/check/merge.h
Outdated
@@ -56,6 +56,32 @@ auto ReplacePrevInstForMerge(Context& context, SemIR::NameScopeId scope_id, | |||
SemIR::NameId name_id, SemIR::InstId new_inst_id) | |||
-> void; | |||
|
|||
// Common information shared by different kinds of named entity, such as classes | |||
// and functions. | |||
struct EntityInfo { |
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.
"EntityInfo" sounds like a generic name for something that's only used by merge. Do you expect additional fields will be added so that it can be used elsewhere? Is this header the right home? Maybe something like DeclParams
might be more descriptive of the use-case?
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.
Renamed to DeclParams
. We can generalize later if needed.
…e#3933) Check that they match across redeclarations. Fix some importing bugs for symbolic bindings which were uncovered when testing this.
Check that they match across redeclarations.
Fix some importing bugs for symbolic bindings which were uncovered when testing this.