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

Stabilise weak_ptr_eq #61797

Open
wants to merge 3 commits into
base: master
from

Conversation

@Thomasdezeeuw
Copy link
Contributor

commented Jun 13, 2019

Implemented in #55987.

Closes #55981.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0422dee9:start=1560421574970719878,finish=1560421575726646528,duration=755926650
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
travis_time:start:tidy
tidy check
[00:04:44] * 576 error codes
[00:04:44] * highest error code: E0731
[00:04:46] tidy error: /checkout/src/liballoc/rc.rs:1545: malformed stability attribute: missing the `since` key
[00:04:46] tidy error: /checkout/src/liballoc/sync.rs:1379: malformed stability attribute: missing the `since` key
[00:04:49] some tidy checks failed
[00:04:49] 
[00:04:49] 
[00:04:49] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:49] 
[00:04:49] 
[00:04:49] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:49] Build completed unsuccessfully in 0:01:13

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Thomasdezeeuw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

I think the failure is unrelated to this pr.

@jonas-schievink jonas-schievink added this to the 1.37 milestone Jun 13, 2019

Show resolved Hide resolved src/liballoc/rc.rs Outdated
Show resolved Hide resolved src/liballoc/sync.rs Outdated

@jonas-schievink jonas-schievink referenced this pull request Jun 13, 2019

Open

Tracking issue for weak_ptr_eq #55981

1 of 2 tasks complete

@Centril Centril added the T-libs label Jun 13, 2019

@chpio

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Can we first change the method into a normal method (taking &self) instead of an associated function (taking this: &Self)?

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

☔️ The latest upstream changes (presumably #61915) made this pull request unmergeable. Please resolve the merge conflicts.

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

r? @sfackler

cc @RalfJung, I'd appreciate a review from you also :)

@Centril Centril assigned sfackler and unassigned kennytm Jul 30, 2019

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

There's not much code to look at, that seems fine. ;)

The docs could be improved though IMO. It first says "Returns true if the two Weaks point to the same value" only to later (in a separate subsection!) note that it also works for Weak::new that do not point to any value -- which seems to contradict the first statement. And moreover, there is no mention of what happens when comparing a Weak that is "dangling" in the sense that upgrade would fail -- there's no value in that case either.

I think Rc::ptr_eq's doc is similarly confusing. I'd argue e.g. that "5" and "5" are the same value, and still the doctest has assert!(!Rc::ptr_eq(&five, &other_five)). Actually it seems like the entire Rc docs use "value" as word for "RcBox instance". IMO that's a very bad choice of words as Rust already uses the same term for a very different meaning.

A better term might be "reference-counted object".

@Thomasdezeeuw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@RalfJung

The docs could be improved though IMO. It first says "Returns true if the two Weaks point to the same value" only to later (in a separate subsection!) note that it also works for Weak::new that do not point to any value -- which seems to contradict the first statement. And moreover, there is no mention of what happens when comparing a Weak that is "dangling" in the sense that upgrade would fail -- there's no value in that case either.

I mostly copied the documentation from Rc::ptr_eq, which always points to a value to the "dangling" case didn't apply there. For Weak I've added the notes section for this. However you did leave a fairly important part of the docs out in your comment, it says:

Returns true if the two Weaks point to the same value (not just values that compare as equal).

(emphasis mine)

The part in between brackets is arguably the most important part, maybe that should be moved to the forefront of the sentence more.

I think Rc::ptr_eq's doc is similarly confusing. I'd argue e.g. that "5" and "5" are the same value, and still the doctest has assert!(!Rc::ptr_eq(&five, &other_five)). Actually it seems like the entire Rc docs use "value" as word for "RcBox instance". IMO that's a very bad choice of words as Rust already uses the same term for a very different meaning.

I agree with this, what do you suggest as concrete improvement?

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

However you did leave a fairly important part of the docs out in your comment, it says:

That was deliberate; as I explained above, values that "compare equal but are not the same" is an odd concept. It indicates that the mathematical integer value "5" might have more to its "identity" that the fact that it is 5? Or maybe integers have provenance?

I know what the text means, but I don't think the parenthetical helps. This is the old problem of comparing values vs. comparing locations (addresses) that hold values, except that it uses exactly the wrong term for what this code really does.

@Thomasdezeeuw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

I agree with your point. What terminology should be used instead? Should "values" not be mentioned at all perhaps and only talk of pointers? I'm really looking for some concrete documentation changes and any help here would be appreciated.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

what do you suggest as concrete improvement?

strawman proposal: "reference-counted object".

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Alternatively we could be talking about comparing things "by their address", as we do in ptr::eq. But then the question remains "the address of what", and that's where something like "reference-counted object" would come up again.

@Thomasdezeeuw

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@RalfJung what do you think about the following?

    /// Returns `true` if the address of the two reference counted objects (the
    /// `Weaks`) are equal.
    ///
    /// # Notes
    ///
    /// Since this compares addresses it means that `Weak::new()` will always
    /// equal each other.

Also do you want me to make the change to Arc::ptr_eq and Rc::ptr_eq as well?

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

The parenthetical is odd -- the object itself isn't weak, only the reference is.

Also do you want me to make the change to Arc::ptr_eq and Rc::ptr_eq as well?

Yes, this should be done consistently.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

And not just that, also docs like this would need adjustment

Gets the number of Weak pointers to this value.

Again, "value" should become "reference-counted object".
Let's get someone from the libs team involved... @SimonSapin, what do you think?

@Thomasdezeeuw

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@RalfJung since the issue of documentation is now gaining in scope and now seems moving to the entire Rc/Weak and Arc/Weak docs, is it still a blocking issue for Weak::ptr_eq? I really would like it to be stabilised in 1.38.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Fair. Maybe open an issue to track improving the docs (by replacing "value" by "reference-counted object" where appropriate)?

My other complaint about the docs was that the "note" contradicts the intro sentence. That is an orthogonal change. Maybe something like

Returns `true` if the two `Weak`s point to the same value (not just values
that compare as equal), or if both don't point to any value
(because they were created with `Weak::new()`).
@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Notice however that I am just the technical reviewer. Stabilization needs to go through libs team FCP, which takes at least 10 days. So I am afraid this already won't make it for the beta. I don't have rfcbot powers though.

@rust-lang/libs could someone start rfcbot? There's some docs bikeshedding still going on but that needn't block FCP.

@Centril Centril modified the milestones: 1.38, 1.39 Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.