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

Move padding checks into derive_util macros #113

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Conversation

djkoloski
Copy link
Member

This moves the padding checks into macros in a new derive_util module and emits implementations with an additional where clause. Some of the proposed names from #109 has been slightly changed to make the emitted error messages more understandable:

error[E0277]: the trait bound `HasPadding<Foo, true>: ShouldBe<false>` is not satisfied
 --> tests/ui-stable/asbytes-illegal.rs:5:10
  |
5 | #[derive(zerocopy::AsBytes)]
  |          ^^^^^^^^^^^^^^^^^ the trait `ShouldBe<false>` is not implemented for `HasPadding<Foo, true>`
  |
  = help: the trait `ShouldBe<VALUE>` is implemented for `HasPadding<T, VALUE>`

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thanks so much for doing this! A few small comments inline, but also: It looks like the new code generates padding errors during an earlier compiler pass than most of the other errors (at least that's my guess as to why many errors are removed in this diff). Could you split up the UI test source files so that we still get test coverage for those errors?

tests/ui/asbytes-illegal.rs Outdated Show resolved Hide resolved
zerocopy-derive/src/lib.rs Show resolved Hide resolved
@djkoloski
Copy link
Member Author

The syn issue mentioned can be fixed by enabling the full feature. I timed a few runs of cargo clean; cargo build -p zerocopy-derive:

"full" off:
    real    0m4.433s
    user    0m8.858s
    sys     0m2.084s

    real    0m4.382s
    user    0m8.990s
    sys     0m1.963s

    real    0m4.447s
    user    0m8.900s
    sys     0m2.032s

    real    0m4.364s
    user    0m8.859s
    sys     0m2.005s

    real    0m4.339s
    user    0m8.678s
    sys     0m2.197s

4.393 +-0.1s

"full" on:
    real    0m5.643s
    user    0m11.430s
    sys     0m2.180s

    real    0m5.840s
    user    0m11.407s
    sys     0m2.192s

    real    0m5.577s
    user    0m11.174s
    sys     0m2.202s

    real    0m5.608s
    user    0m11.192s
    sys     0m2.183s

    real    0m5.720s
    user    0m11.271s
    sys     0m2.243s

5.6776 =- 0.23s

So it looks like enabling full adds about 1.2s to each build of zerocopy-derive (+29.2% for this package alone). Should we continue with a workaround or enable the full feature?

@joshlf
Copy link
Member

joshlf commented Nov 1, 2022

Yeah let's stick to the hack; it's pretty minor, and that's a lot of compilation time to add.

@joshlf
Copy link
Member

joshlf commented Nov 1, 2022

It looks like the new code generates padding errors during an earlier compiler pass than most of the other errors (at least that's my guess as to why many errors are removed in this diff). Could you split up the UI test source files so that we still get test coverage for those errors?

Looks like this wasn't addressed yet - just a heads up in case you're waiting for a review on the latest changes. If you were planning on addressing this later, then just disregard this comment.

zerocopy-derive/tests/ui/padding_check.rs Outdated Show resolved Hide resolved
zerocopy-derive/tests/ui/padding_check.rs Outdated Show resolved Hide resolved
zerocopy-derive/tests/ui/padding_check.stderr Outdated Show resolved Hide resolved
zerocopy-derive/tests/ui/late_compile_pass.rs Outdated Show resolved Hide resolved
zerocopy-derive/tests/ui/struct.rs Show resolved Hide resolved
@joshlf joshlf merged commit 6bd9d7d into google:main Nov 3, 2022
joshlf pushed a commit that referenced this pull request Aug 3, 2023
Move the padding checks into macros in a new `derive_util`
module and emit implementations with an additional `where`
clause.

Closes #109 

Co-authored-by: David Koloski <[email protected]>
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