dicej opened PR #4359 from component-type-derive-more
to main
:
This is the second stage of implementing #4308. It adds support for deriving
variant, enum, and union impls forComponentType
,Lift
, andLower
. It
also fixes derived record impls for genericstruct
s, which I had intended to
support in my previous commit, but forgot to test.Signed-off-by: Joel Dice <joel.dice@fermyon.com>
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Is this mostly to just simplify this for now where you don't have to worry about 16-vs-8 bit loads?
jameysharp created PR review comment:
union
could reasonably acceptsyn::Fields::Unit
cases:X
should just be treated likeX(())
. I'm thinking you could simplify a bunch of this by building aVec<VariantCase>
like inexpand_variant
, even for enums and unions. Only in theenum
case do you need to check that all cases havety: None
.Similarly, I think all the generated code would be correct for
Lift
andLower
if you used theexpand_variant
implementation for all three cases.expand_enum
doesn't strictly need to handle non-None
case types, but those branches will just never be taken.The only thing that's actually different in an interesting way is the type-checking implementation for
ComponentType
. Unless I've missed something?
jameysharp submitted PR review.
dicej created PR review comment:
Did you notice that both
expand_variant
andexpand_union
defer toExpander::expand_variant
? So the duplication is already pretty minimal. But yeah,expand_variant
andexpand_union
could themselves be combined.
dicej submitted PR review.
dicej created PR review comment:
Yeah, I was just keeping it simple to start with. I was also not sure what the theoretical max was for the component model. 2^32? More?
dicej submitted PR review.
dicej edited PR review comment.
jameysharp submitted PR review.
jameysharp created PR review comment:
I did notice that, although it took me a few read-throughs. :laughing: So yes, I'm suggesting combining
Expander::expand_variant
withExpander::expand_enum
, except that I know you'll need to take a little care with the implementation forComponentType
. But I'm also suggesting combining the top-levelexpand_variant
,expand_union
, andexpand_enum
.
jameysharp submitted PR review.
jameysharp created PR review comment:
The Canonical ABI defines that in the
discriminant_type
function, and the current maximum is 2^32.
dicej created PR review comment:
I see -- I'll give that a shot.
dicej submitted PR review.
dicej updated PR #4359 from component-type-derive-more
to main
.
dicej submitted PR review.
dicej created PR review comment:
Yeah, that slimmed it down a lot. Great suggestion!
jameysharp submitted PR review.
alexcrichton merged PR #4359.
Last updated: Dec 23 2024 at 12:05 UTC