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 bindings compilation #184

Merged
merged 2 commits into from
Oct 7, 2023
Merged

Fix bindings compilation #184

merged 2 commits into from
Oct 7, 2023

Conversation

commial
Copy link
Contributor

@commial commial commented Oct 6, 2023

The idiomatic way to test for null arguments is to use Option<type>.

For now, bindgen "forgets" what the inner type is in the following configuration:

type X = ...;
fn (x : Option<X>) ...

An opaque Option_X will be generated.

To avoid this behavior and conserve the actual documentation, this PR:

  • introduces types for X and Option<X>
  • these types are duplicated, as bindgen does not introspect the Option<type alias>
  • to avoid API breaks and using an Option internally, the code uses a Rust FFI guarantee: Option<X> and X are represented the same way

@commial commial added the refactoring Code refactoring label Oct 6, 2023
@commial
Copy link
Contributor Author

commial commented Oct 6, 2023

ping @mtth-bfft

@commial commial mentioned this pull request Oct 6, 2023
@mtth-bfft
Copy link

It would probably be best for documentation purposes to add a link to the underlying issue (mozilla/cbindgen#326) in the source comment, both for future understanding and so everyone can check if the issue made progress and we can clean up this workaround eventually

It's more complex with twice the number of typedefs, but I agree it's better than #[allow(clippy::fn_null_check)] . Lgtm, thanks for adding this fix ! 👍

@commial
Copy link
Contributor Author

commial commented Oct 7, 2023

Thanks for the ref, I've added it to the corresponding commit message.

@commial commial merged commit f269c5e into master Oct 7, 2023
24 checks passed
@commial commial deleted the fix-bindings branch October 7, 2023 13:19
@github-actions
Copy link

github-actions bot commented Oct 7, 2023

Benchmark for 1d5032b

Click to view benchmark
Test Base PR %
chunk_size_decompress_mutilfiles_random/Layers(0x0)/1024 922.5±55.89ns 980.0±87.07ns +6.23%
chunk_size_decompress_mutilfiles_random/Layers(0x0)/1048576 105.9±5.99µs 112.4±6.07µs +6.14%
chunk_size_decompress_mutilfiles_random/Layers(0x0)/16777216 1653.2±33.86µs 1682.1±47.48µs +1.75%
chunk_size_decompress_mutilfiles_random/Layers(0x0)/65536 7.2±0.62µs 7.5±0.37µs +4.17%
chunk_size_decompress_mutilfiles_random/Layers(COMPRESS)/1024 744.0±408.06µs 747.2±408.51µs +0.43%
chunk_size_decompress_mutilfiles_random/Layers(COMPRESS)/1048576 32.8±0.06ms 33.6±0.10ms +2.44%
chunk_size_decompress_mutilfiles_random/Layers(COMPRESS)/16777216 262.0±0.28ms 266.0±0.17ms +1.53%
chunk_size_decompress_mutilfiles_random/Layers(COMPRESS)/65536 6.0±0.01ms 6.0±0.01ms 0.00%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT | COMPRESS)/1024 1094.3±598.63µs 1097.7±603.24µs +0.31%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT | COMPRESS)/1048576 36.4±0.05ms 36.8±0.05ms +1.10%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT | COMPRESS)/16777216 390.3±0.33ms 395.2±0.83ms +1.26%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT | COMPRESS)/65536 10.2±4.89ms 10.3±4.92ms +0.98%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT)/1024 1112.8±322.12µs 1043.7±289.79µs -6.21%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT)/1048576 11.2±0.01ms 11.2±0.28ms 0.00%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT)/16777216 161.7±0.69ms 162.2±0.16ms +0.31%
chunk_size_decompress_mutilfiles_random/Layers(ENCRYPT)/65536 1953.3±47.87µs 1856.3±49.72µs -4.97%
failsafe_multiple_layers_repair/Layers(0x0)/4194304 109.4±0.06ms 109.3±0.08ms -0.09%
failsafe_multiple_layers_repair/Layers(COMPRESS)/4194304 177.2±0.13ms 176.9±0.21ms -0.17%
failsafe_multiple_layers_repair/Layers(ENCRYPT | COMPRESS)/4194304 187.4±0.35ms 187.1±0.42ms -0.16%
failsafe_multiple_layers_repair/Layers(ENCRYPT)/4194304 121.3±0.18ms 121.1±0.19ms -0.16%
reader_multiple_layers_multiple_block_size/Layers(0x0)/1024 163.6±7.98ns 170.5±5.71ns +4.22%
reader_multiple_layers_multiple_block_size/Layers(0x0)/1048576 103.4±10.10µs 106.7±8.15µs +3.19%
reader_multiple_layers_multiple_block_size/Layers(0x0)/16777216 1695.5±40.46µs 1669.2±25.40µs -1.55%
reader_multiple_layers_multiple_block_size/Layers(0x0)/65536 6.4±0.84µs 6.8±0.64µs +6.25%
reader_multiple_layers_multiple_block_size/Layers(COMPRESS)/1024 1751.0±2249.17ns 1738.9±2233.44ns -0.69%
reader_multiple_layers_multiple_block_size/Layers(COMPRESS)/1048576 131.9±2.84µs 130.6±1.78µs -0.99%
reader_multiple_layers_multiple_block_size/Layers(COMPRESS)/16777216 197.3±0.18ms 200.6±0.26ms +1.67%
reader_multiple_layers_multiple_block_size/Layers(COMPRESS)/65536 153.8±182.75µs 154.0±182.41µs +0.13%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/1024 22.3±3.71µs 21.9±3.65µs -1.79%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/1048576 23.1±0.03ms 22.6±0.05ms -2.16%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/16777216 395.7±2.04ms 392.6±0.43ms -0.78%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/65536 1392.7±187.19µs 1365.0±185.68µs -1.99%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT)/1024 10.0±0.58µs 9.7±0.55µs -3.00%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT)/1048576 10.4±0.29ms 10.1±0.29ms -2.88%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT)/16777216 169.3±0.10ms 164.7±1.75ms -2.72%
reader_multiple_layers_multiple_block_size/Layers(ENCRYPT)/65536 640.5±32.66µs 621.4±32.07µs -2.98%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(0x0)/1024 756.4±44.56ns 744.3±41.47ns -1.60%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(0x0)/1048576 101.9±5.81µs 104.0±2.67µs +2.06%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(0x0)/16777216 1672.2±41.41µs 1696.6±30.52µs +1.46%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(0x0)/65536 7.4±0.23µs 7.4±0.34µs 0.00%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(COMPRESS)/1024 18.4±0.32µs 18.3±0.15µs -0.54%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(COMPRESS)/1048576 16.2±0.03ms 16.3±0.03ms +0.62%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(COMPRESS)/16777216 262.2±0.19ms 263.6±0.29ms +0.53%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(COMPRESS)/65536 1025.4±6.69µs 1033.0±7.06µs +0.74%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT | COMPRESS)/1024 26.8±0.21µs 26.9±0.26µs +0.37%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT | COMPRESS)/1048576 24.2±0.04ms 24.4±0.08ms +0.83%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT | COMPRESS)/16777216 393.3±0.40ms 393.5±0.36ms +0.05%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT | COMPRESS)/65536 1526.6±11.15µs 1527.1±11.00µs +0.03%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT)/1024 11.8±0.11µs 11.6±0.11µs -1.69%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT)/1048576 10.1±0.01ms 10.1±0.07ms 0.00%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT)/16777216 161.9±0.11ms 161.8±0.11ms -0.06%
reader_multiple_layers_multiple_block_size_multifiles_linear/Layers(ENCRYPT)/65536 645.9±2.04µs 633.6±0.86µs -1.90%
writer_multiple_layers_multiple_block_size/Layers(0x0)/1024 13.5±0.09µs 13.5±0.12µs 0.00%
writer_multiple_layers_multiple_block_size/Layers(0x0)/1048576 13.5±0.16ms 13.5±0.15ms 0.00%
writer_multiple_layers_multiple_block_size/Layers(0x0)/16777216 216.5±0.99ms 216.8±1.02ms +0.14%
writer_multiple_layers_multiple_block_size/Layers(0x0)/65536 841.9±11.17µs 842.7±11.39µs +0.10%
writer_multiple_layers_multiple_block_size/Layers(COMPRESS)/1024 21.4±0.44µs 21.6±0.51µs +0.93%
writer_multiple_layers_multiple_block_size/Layers(COMPRESS)/1048576 26.7±1.39ms 27.0±1.45ms +1.12%
writer_multiple_layers_multiple_block_size/Layers(COMPRESS)/16777216 666.4±2.40ms 671.8±2.33ms +0.81%
writer_multiple_layers_multiple_block_size/Layers(COMPRESS)/65536 1326.7±29.10µs 1341.1±32.27µs +1.09%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/1024 21.4±0.56µs 21.5±0.54µs +0.47%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/1048576 28.6±1.97ms 29.1±1.89ms +1.75%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/16777216 791.6±2.13ms 800.3±2.52ms +1.10%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT | COMPRESS)/65536 1331.1±26.79µs 1345.8±28.12µs +1.10%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT)/1024 24.6±0.07µs 24.3±0.08µs -1.22%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT)/1048576 23.9±0.09ms 23.7±0.09ms -0.84%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT)/16777216 382.2±0.88ms 378.7±0.83ms -0.92%
writer_multiple_layers_multiple_block_size/Layers(ENCRYPT)/65536 1489.9±7.27µs 1476.1±7.23µs -0.93%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants