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

Fix unknown id Type for EnumExt::deku_id() #398

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

wcampbell0x2a
Copy link
Collaborator

@wcampbell0x2a wcampbell0x2a commented Jan 3, 2024

Remove unwrap causing the following error when the EnumExt tried to infer what type is was to return. An attribute could be added in the future to aid this, but for now we will just not emit the deku_id() for this type of enum. I decided in other causes (which aren't tested), to also remove the error and just not emit the function also.

help: message: called Result::unwrap() on an Err value: Error("expected ,")

Closes #397

Remove unwrap causing the following error when the EnumExt tried to infer
what type is was to return. An attribute could be added in the future to aid this,
but for now we will just not emit the deku_id() for this type of enum.
I decided in other causes (which aren't tested), to also remove the error
and just not emit the function also.

 help: message: called `Result::unwrap()` on an `Err` value: Error("expected `,`")

See #397
Copy link

github-actions bot commented Jan 3, 2024

Benchmark for 65065c4

Click to view benchmark
Test Base PR %
deku_read_bits 1111.2±24.21ns 1156.7±25.73ns +4.09%
deku_read_byte 22.5±0.62ns 22.6±0.64ns +0.44%
deku_read_enum 9.4±0.10ns 9.2±0.25ns -2.13%
deku_read_vec 54.2±0.96ns 52.8±1.41ns -2.58%
deku_write_bits 107.1±2.87ns 108.1±6.18ns +0.93%
deku_write_byte 121.6±5.47ns 120.5±4.21ns -0.90%
deku_write_enum 83.1±2.54ns 81.8±2.75ns -1.56%
deku_write_vec 3.0±0.06µs 3.0±0.08µs 0.00%

@sharksforarms
Copy link
Owner

Fair enough. From a code-gen perspective, I'm not sure if there's a way of getting the type? probably some hackery via walking the struct/enum, etc.

We currently have type to specify the variant type... wonder if we could extend the use of it to include this case

@wcampbell0x2a
Copy link
Collaborator Author

Fair enough. From a code-gen perspective, I'm not sure if there's a way of getting the type? probably some hackery via walking the struct/enum, etc.

We currently have type to specify the variant type... wonder if we could extend the use of it to include this case

Right now we don't allow id and type(to be id_type in the future). I don't know if I have the time/patience right now to figure out syn invocations to parse that xD

@sharksforarms
Copy link
Owner

sharksforarms commented Jan 3, 2024

Fair enough. From a code-gen perspective, I'm not sure if there's a way of getting the type? probably some hackery via walking the struct/enum, etc.
We currently have type to specify the variant type... wonder if we could extend the use of it to include this case

Right now we don't allow id and type(to be id_type in the future). I don't know if I have the time/patience right now to figure out syn invocations to parse that xD

Yeah I agree, I don't favor that approach. You're right, currently we can't use id and type together, what if we could?
id - use id to select variant
type - use type to read/write variant of type type
id + type - use id to select variant with type type

sharksforarms
sharksforarms previously approved these changes Jan 6, 2024
Copy link
Owner

@sharksforarms sharksforarms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a good solution for now, I created #400 to revisit in the future

Copy link

github-actions bot commented Jan 6, 2024

Benchmark for dc44332

Click to view benchmark
Test Base PR %
deku_read_bits 1199.3±16.88ns 1187.8±25.00ns -0.96%
deku_read_byte 23.3±0.34ns 23.3±0.93ns 0.00%
deku_read_enum 9.3±0.08ns 9.5±0.22ns +2.15%
deku_read_vec 53.5±0.80ns 55.0±1.33ns +2.80%
deku_write_bits 109.0±1.35ns 109.7±3.33ns +0.64%
deku_write_byte 123.6±3.85ns 123.3±3.02ns -0.24%
deku_write_enum 87.6±3.18ns 83.3±1.78ns -4.91%
deku_write_vec 3.1±0.08µs 3.0±0.05µs -3.23%

@wcampbell0x2a wcampbell0x2a merged commit f568eca into master Jan 16, 2024
15 checks passed
@wcampbell0x2a wcampbell0x2a deleted the fix-396-enum-ext-no-id branch January 16, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help passing context around
2 participants