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

Remove extension_trait #1006

Merged
merged 11 commits into from Mar 16, 2022
Merged

Conversation

nnethercote
Copy link
Contributor

@yoshuawuyts says that the documentation generated by extension_trait is no longer necessary, which opens the door to removing the extension_trait macro. This PR does that, in a series of small steps. When combined with the improvements in #1005, the time for a full cargo check has reduced by about 55%, and the time for an incremental cargo check rebuild has reduced by about 75%.

Two of the rules have `(+ $lt:lifetime)?` that is not used on the RHS
and serves no useful purpose. This commit removes it.
This is the `@doc` rules, the shim trait impls, and the imports.
The body and doc comment are no longer used.
The remaining type requires the square brackets (for now) because a `ty`
cannot immediately precede a `$(tt)*`.
At this point, `extension_trait` is basically an expensive no-op. This
commit removes it. The next commit will adjust the indentation.
This commit only affects whitespace; `git diff -w` for it is empty.
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

cc @lqd

@nnethercote
Copy link
Contributor Author

Oh, src/lib.rs has #![recursion_limit = "2048"], that can probably be removed now.

Now that `extension_trait!` is gone, an increased limit isn't necessary.
@nnethercote
Copy link
Contributor Author

Oh, src/lib.rs has #![recursion_limit = "2048"], that can probably be removed now.

It can, I just pushed an additional commit to remove it.

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Tested this locally, and woah this compiles quickly! -- incredibly excited about this. Thanks so much @nnethercote!

@yoshuawuyts yoshuawuyts merged commit 7e455db into async-rs:master Mar 16, 2022
@lnicola
Copy link

lnicola commented Mar 16, 2022

@yoshuawuyts just curious, what's "quickly" here? I might have tested incorrectly, but async-std without its dependencies builds in about 2 seconds for me.

@nnethercote nnethercote deleted the rm-extension_trait branch March 16, 2022 11:49
@yoshuawuyts
Copy link
Contributor

@lnicola oh, I meant "quickly" compared to compile times before this change. In benchmarks this change showed a >50% improvement in compile times, and trying it locally it def felt noticeably faster!

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.

None yet

3 participants