Skip to content
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

Upstream code gen patches #20

Open
i-norden opened this issue Jun 25, 2021 · 2 comments
Open

Upstream code gen patches #20

i-norden opened this issue Jun 25, 2021 · 2 comments

Comments

@i-norden
Copy link
Collaborator

i-norden commented Jun 25, 2021

While testing these codecs in #18 we've uncovered some issues with the code gen, namely:

  1. Union representation types are generated (e.g. _Child__Repr) but not used by other generated types, they use the default representation (e.g. _Child) which are a map keyed with the name of the types the Union can hold (this might only be an issue with my expectations).

  2. Issue with AssembleValue() switch statements for union Assemblers (e.g. _TrieNode__Assembler; ma.ca cases are off by one).

  3. Needed to manually change some union type fields to pointers to allow compilation of recursive types.

@i-norden i-norden added this to To Do in Ethereum IPLD via automation Jun 25, 2021
@i-norden i-norden moved this from To Do to In Progress in Ethereum IPLD Jun 25, 2021
@warpfork
Copy link

warpfork commented Jul 2, 2021

Gave this a quick look --

  1. might indeed be an expectation check. If you're just looking at the generated code and thinking it looks funny -- it does that sometimes :) Most of the time, the types compose using the non-repr form, and there's a "cast" to the repr form on demand. The "cast" is free because the two types have the same memory layout, and is actually optimized into nothing by the compiler -- it just gives us access to different methodsets and the ability to apply them to the same memory.

  2. that sounds like it might be a real bug, if you're sure it's behaving wrong please do open a bug upstream :)

  3. is probably fixable with configuration -- commented in the other issue.

It's not impossible there are bugs in situation 1, too, but if you are just eyeballing code... probably better to run it and see if it's behaviourally wrong.

@i-norden
Copy link
Collaborator Author

i-norden commented Aug 4, 2021

Hey @warpfork thanks for getting back to me on this! I missed the notification. Looking into this further today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Ethereum IPLD
In Progress
Development

No branches or pull requests

2 participants