Accept generic parameter lists in class declarations. #3933
Conversation
Check that they match across redeclarations.
| class Generic(T:! type) { | ||
| } | ||
|
|
||
| // --- fail_invalid.carbon |
There was a problem hiding this comment.
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.
| if (EntityHasParamError(context, new_entity) || | ||
| EntityHasParamError(context, prev_entity)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
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.
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.
"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.
Renamed to DeclParams. We can generalize later if needed.
Check that they match across redeclarations.
Fix some importing bugs for symbolic bindings which were uncovered when testing this.