-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Initial support for empty named constraints #6245
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
Conversation
|
Going to toss this toward @jonmeow for review. A lot of it is just boilerplate. I think the interesting question is what is the best way to share code between interface and constraint. |
toolchain/parse/testdata/generics/named_constraint/template_constraint.carbon
Show resolved
Hide resolved
toolchain/check/testdata/named_constraint/fail_invalid_method.carbon
Outdated
Show resolved
Hide resolved
toolchain/check/testdata/named_constraint/fail_invalid_method.carbon
Outdated
Show resolved
Hide resolved
|
I did a quick pass, but I'm going to be off next week so re-adding @geoffromer who got the initial autoassign. |
Oops, thanks, I removed him as I thought something went wrong. Re-added back. But @geoffromer you can hold off reviewing until I address this feedback. I will ping you when it's ready. |
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.
But @geoffromer you can hold off reviewing until I address this feedback. I will ping you when it's ready.
Sounds good. (Replying here so the bot stops pinging me :-) )
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.
PTAL
toolchain/check/testdata/named_constraint/fail_invalid_method.carbon
Outdated
Show resolved
Hide resolved
toolchain/check/testdata/named_constraint/fail_invalid_method.carbon
Outdated
Show resolved
Hide resolved
toolchain/check/testdata/named_constraint/fail_todo_unspecified.carbon
Outdated
Show resolved
Hide resolved
toolchain/check/testdata/named_constraint/fail_invalid_method.carbon
Outdated
Show resolved
Hide resolved
toolchain/parse/testdata/generics/named_constraint/template_constraint.carbon
Show resolved
Hide resolved
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.
PTAL
| template <typename EntityT> | ||
| requires std::same_as<EntityT, SemIR::Interface> | ||
| static auto TryGetEntity(Context& context, SemIR::Inst inst) | ||
| -> const SemIR::EntityWithParamsBase* { | ||
| if (auto decl = inst.TryAs<SemIR::InterfaceDecl>()) { | ||
| return &context.interfaces().Get(decl->interface_id); | ||
| } else { | ||
| return nullptr; | ||
| } | ||
| } | ||
|
|
||
| template <typename EntityT> | ||
| requires std::same_as<EntityT, SemIR::NamedConstraint> | ||
| static auto TryGetEntity(Context& context, SemIR::Inst inst) | ||
| -> const SemIR::EntityWithParamsBase* { | ||
| if (auto decl = inst.TryAs<SemIR::NamedConstraintDecl>()) { | ||
| return &context.named_constraints().Get(decl->named_constraint_id); | ||
| } else { | ||
| return nullptr; | ||
| } | ||
| } |
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.
Optional: I think you can make these into a conventional overload set with something like this:
| template <typename EntityT> | |
| requires std::same_as<EntityT, SemIR::Interface> | |
| static auto TryGetEntity(Context& context, SemIR::Inst inst) | |
| -> const SemIR::EntityWithParamsBase* { | |
| if (auto decl = inst.TryAs<SemIR::InterfaceDecl>()) { | |
| return &context.interfaces().Get(decl->interface_id); | |
| } else { | |
| return nullptr; | |
| } | |
| } | |
| template <typename EntityT> | |
| requires std::same_as<EntityT, SemIR::NamedConstraint> | |
| static auto TryGetEntity(Context& context, SemIR::Inst inst) | |
| -> const SemIR::EntityWithParamsBase* { | |
| if (auto decl = inst.TryAs<SemIR::NamedConstraintDecl>()) { | |
| return &context.named_constraints().Get(decl->named_constraint_id); | |
| } else { | |
| return nullptr; | |
| } | |
| } | |
| template <typename T> | |
| struct Tag {}; | |
| static auto TryGetEntity(Tag<SemIR::Interface>, Context& context, SemIR::Inst inst) | |
| -> const SemIR::EntityWithParamsBase* { | |
| if (auto decl = inst.TryAs<SemIR::InterfaceDecl>()) { | |
| return &context.interfaces().Get(decl->interface_id); | |
| } else { | |
| return nullptr; | |
| } | |
| } | |
| static auto TryGetEntity(Tag<SemIR::NamedConstraint>, Context& context, SemIR::Inst inst) | |
| -> const SemIR::EntityWithParamsBase* { | |
| if (auto decl = inst.TryAs<SemIR::NamedConstraintDecl>()) { | |
| return &context.named_constraints().Get(decl->named_constraint_id); | |
| } else { | |
| return nullptr; | |
| } | |
| } |
Then the callsite looks like TryGetEntity(Tag<EntityT>{}, context, existing_decl_inst).
This technique trades off a slightly more awkward interface for a more straightforward implementation, so I'd be more hesitant to do it with a public function, but it can be a good fit for implementation details within a .cpp file.
|
My reply here isn't showing easily on the convo tab so surfacing it for you. |
Type check named constraint decls and definitions. We don't correctly error if you put a
fninside them. There is no support forrequireoraliasyet, so there's nothing useful you can do with them yet.We have attempted to share code between
interfaceandconstraintas they are quite similar. First by splitting out some of handle_interface.cpp to a separate file. Second by sharing some code paths when you want a facet type from either one, as they both turn into a facet type.