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 error for invalid deku_id generation on generic enum #411

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

wcampbell0x2a
Copy link
Collaborator

  • Add/Fixup lifetime for generated code for enum ext id finding. Lifetime __deku needs to be added to the deku_id_type, and the impl.

NOTE: I'm not sure if __deku the the best answer, and now ignoring generics is more of a problem of me fixing the problem and the time I have to fix it in a day.

See #408, this also deals with #400

Copy link

Benchmark for 620f4ef

Click to view benchmark
Test Base PR %
deku_read_bits 1115.7±17.37ns 1147.6±16.91ns +2.86%
deku_read_byte 20.2±0.27ns 20.2±0.40ns 0.00%
deku_read_enum 9.3±0.07ns 9.4±0.12ns +1.08%
deku_read_vec 53.2±0.87ns 53.7±0.80ns +0.94%
deku_write_bits 114.3±1.25ns 114.3±1.62ns 0.00%
deku_write_byte 134.5±11.63ns 129.3±5.69ns -3.87%
deku_write_enum 93.6±3.21ns 91.8±1.66ns -1.92%
deku_write_vec 3.1±0.06µs 3.1±0.04µs 0.00%

sharksforarms
sharksforarms previously approved these changes Apr 29, 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.

lgtm, a test case would be good for this too

Add/Fixup lifetime for generated code for enum ext id finding.
Lifetime __deku needs to be added to the deku_id_type, and the impl.
@wcampbell0x2a wcampbell0x2a force-pushed the fix-required-clone-for-deku-enum-ext branch from e32a52f to 055f9a9 Compare April 30, 2024 01:57
@wcampbell0x2a wcampbell0x2a changed the title Fix error for non Copy enum ids Fix error for invalid deku_id generation on generic enum Apr 30, 2024
Copy link

Benchmark for 2df9a4d

Click to view benchmark
Test Base PR %
deku_read_bits 1208.2±12.66ns 1172.8±50.33ns -2.93%
deku_read_byte 5.5±0.12ns 5.5±0.06ns 0.00%
deku_read_enum 2.9±0.07ns 2.9±0.06ns 0.00%
deku_read_vec 35.4±1.63ns 35.0±0.34ns -1.13%
deku_write_bits 202.4±3.80ns 199.3±5.89ns -1.53%
deku_write_byte 22.0±0.30ns 22.0±0.10ns 0.00%
deku_write_enum 23.5±0.27ns 23.8±1.89ns +1.28%
deku_write_vec 290.0±2.63ns 288.9±2.39ns -0.38%

@wcampbell0x2a
Copy link
Collaborator Author

lgtm, a test case would be good for this too

Added!

@wcampbell0x2a wcampbell0x2a added this to the 0.17.0 milestone Apr 30, 2024
@wcampbell0x2a wcampbell0x2a merged commit b6a46d5 into master Apr 30, 2024
11 of 13 checks passed
@wcampbell0x2a wcampbell0x2a deleted the fix-required-clone-for-deku-enum-ext branch April 30, 2024 13:18
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.

2 participants