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

chore: reduce dependency to pin-utils #2929

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Feb 26, 2025

This refers to #2924.

cc @taiki-e as for "it is reasonable to remove it in v0.4", IIUC I can open a follow-up PR to remove this macro and update the corresponding related usage, only we don't mark that PR as 0.3-backport?

BTW, if there is anything I can help in backporting, please let me know. In my experience, it means opening a PR against branch 0.3 and do the same change.

@rustbot rustbot added A-future Area: futures::future S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2025
@tisonkun tisonkun force-pushed the reduce-dep-to-pin-utils branch 2 times, most recently from 6bec98d to 9c206fe Compare February 26, 2025 23:19
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun force-pushed the reduce-dep-to-pin-utils branch from 9c206fe to 6d7ea3e Compare February 26, 2025 23:22
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

futures-util/src/future/future/mod.rs (which is a module for Future trait related things) is not the proper place to put this macro, so perhaps it is better to create futures-util/src/macros.rs and put it there.

// ever again.
#[allow(unused_mut)]
let mut $x = unsafe {
::core::pin::Pin::new_unchecked(&mut $x)
Copy link
Member

Choose a reason for hiding this comment

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

This should refer $crate::__private::Pin instead of ::core::pin::Pin, as pin-utils does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any rationale here?

I'm afraid that, as futures_util::__private is guarded by async-await flag, it would create an extra flag gate to use this name.

Copy link
Member

Choose a reason for hiding this comment

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

Any rationale here?

See rust-lang/pin-utils#26 (comment).

I'm afraid that, as futures_util::__private is guarded by async-await flag, it would create an extra flag gate to use this name.

There is no problem with removing the cfg from __private module.
That cfg just indicates who the current user is, nothing special.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Let me use $crate::__private and remove the cfg.

Maybe tomorrow to trigger CI with the new toolchain that fixes the current CI failure.

@taiki-e taiki-e added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author A-macro Area: macro related and removed A-future Area: futures::future S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2025
@rustbot rustbot added the A-future Area: futures::future label Mar 22, 2025
@tisonkun
Copy link
Contributor Author

@taiki-e Thanks for your review! Comments addressed.

... except the one about $crate::__private::Pin that I found the __private mod is guarded by "async-await" flag and being a bit uncertain to use.

@tisonkun tisonkun requested a review from taiki-e March 22, 2025 16:10
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun force-pushed the reduce-dep-to-pin-utils branch from c5f4c95 to 3629b06 Compare March 22, 2025 16:11
@tisonkun
Copy link
Contributor Author

error: unused attribute `<cfg_attr>`
   --> futures-util/src/stream/stream/mod.rs:181:1
    |
181 | #[cfg_attr(target_os = "none", cfg(target_has_atomic = "ptr"))]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: the built-in attribute `<cfg_attr>` will be ignored, since it's applied to the macro invocation `delegate_all`
   --> futures-util/src/stream/stream/mod.rs:183:1
    |
183 | delegate_all!(
    | ^^^^^^^^^^^^
    = note: `-D unused-attributes` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_attributes)]`

Doesn't seem related .. Unsure what happen.

@taiki-e
Copy link
Member

taiki-e commented Mar 22, 2025

error: unused attribute `<cfg_attr>`
   --> futures-util/src/stream/stream/mod.rs:181:1
    |
181 | #[cfg_attr(target_os = "none", cfg(target_has_atomic = "ptr"))]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: the built-in attribute `<cfg_attr>` will be ignored, since it's applied to the macro invocation `delegate_all`
   --> futures-util/src/stream/stream/mod.rs:183:1
    |
183 | delegate_all!(
    | ^^^^^^^^^^^^
    = note: `-D unused-attributes` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_attributes)]`

Doesn't seem related .. Unsure what happen.

This is rust-lang/rust#138779.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-future Area: futures::future A-macro Area: macro related S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants