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.

Sign up for GitHub

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

Jump to bottom

Attach follows_from() before constructing future #2686

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

Attach follows_from() before constructing future #2686

wants to merge 2 commits into from

Conversation

matthiasbeyer
Copy link

@matthiasbeyer matthiasbeyer commented Aug 17, 2023 •
edited
Loading

Motivation

See #2650

Solution

Moving loop that makes the attribute span follows_from() other attribute spans before the creation of the future to remove the implicit Copy requirement for attributes.

CC @hawkw does this look right to you? How and where can I add a testcase for this ( I am a first time contributor to this project)?

CC @TheNeikos (submitted the issue mentioned above with me)

@hawkw
Copy link
Member

hawkw commented Aug 17, 2023

Thanks for working on this!

CC @hawkw does this look right to you? How and where can I add a testcase for this ( I am a first time contributor to this project)?

There are tests for the #[instrument] macro's follows_from support in this file. I would add a test where a non-Copy parameter is used in follows_from --- we probably start by copying the example provided in #2650 into a test in that file, I think. Since this is a compile error rather than a runtime error, we would expect the test to not compile on the master branch, and we would expect it to compile successfully on this branch.

Let me know if you have any more questions!

@matthiasbeyer
Copy link
Author

Does this look like it is going in the right direction to you?

hawkw
hawkw approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@hawkw hawkw 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 good to me, thank you!

@matthiasbeyer
Copy link
Author

I think we can make the Id in NonCopyContext an Option<Id>, because the actual id is irrelevant for the testcase.

@hawkw hawkw enabled auto-merge (squash) August 29, 2023 21:44
auto-merge was automatically disabled September 8, 2023 11:53

Head branch was pushed to by a user without write access

hawkw
hawkw approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me, provided CI passes!

@matthiasbeyer
Copy link
Author

It seems that the only thing left here is formatting.
I hope I can get this squashed tomorrow. If not, I am off for one week (see you all at Rustconf), so this has to wait for after I return home. 😂

@hawkw
Copy link
Member

hawkw commented Sep 8, 2023

@matthiasbeyer if you don't mind selecting "Allow changes from maintainers", I'm happy to pull the branch and run rustfmt on my machine, and then I can merge it? Otherwise, pushing a commit to fix rustfmt tomorrow is good with me!

@matthiasbeyer
Attach follows_from() before constructing future
261539b 
This patch moves the calls to `follows_from()` on the attribute span
_before_ the construction of the future, so that the arguments that are
moved into the future are used before that move.

Before this, the attributes needed to be `Copy`, so they could be moved
into the future and then be used in the `follows_from()` call.
This patch removes this (implicit) requirement.

Suggested-by: Eliza Weisman <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
@matthiasbeyer
Add test for follows-from-noncopy-type
d0055ab 
Signed-off-by: Matthias Beyer <[email protected]>
@matthiasbeyer
Copy link
Author

matthiasbeyer commented Sep 9, 2023 •
edited
Loading

Squashed the changes and ensured the changes in the commits are formatted. I'll also enable edits! Was already enabled. 😄 👍

@zohnannor
Copy link

Hi! Can this be merged soon?

@TheNeikos
Copy link

Would this still be useful?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@davidbarsky davidbarsky Awaiting requested review from davidbarsky davidbarsky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthiasbeyer @hawkw @zohnannor @TheNeikos