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

Followup for #46112. #46838

Merged
merged 1 commit into from Dec 22, 2017

Conversation

Projects
None yet
7 participants
@pnkfelix
Member

pnkfelix commented Dec 19, 2017

Sorting by crate-num should ensure that we favor std::foo::bar over
any_other_crate::foo::bar.

Interestingly, this change had a much larger impact on our internal
test suite than PR #46708 (which was my original fix to #46112).

Followup for #46112.
Sorting by crate-num should ensure that we favor `std::foo::bar` over
`any_other_crate::foo::bar`.

Interestingly, *this* change had a much larger impact on our internal
test suite than PR #46708 (which was my original fix to #46112).
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 19, 2017

Collaborator

r? @estebank

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

Collaborator

rust-highfive commented Dec 19, 2017

r? @estebank

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

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 19, 2017

Contributor

@bors r+

Contributor

estebank commented Dec 19, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 19, 2017

Contributor

📌 Commit aa030dd has been approved by estebank

Contributor

bors commented Dec 19, 2017

📌 Commit aa030dd has been approved by estebank

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Dec 21, 2017

reverted tests to cope with "regression" back to printing leading `co…
…re::`

The reason we see `core::` even after visiting the `std` crate first
is the special case code that looks like this:

```rust
    Entry::Occupied(mut entry) => {
        // If `child` is defined in crate `cnum`, ensure
        // that it is mapped to a parent in `cnum`.
        if child.krate == cnum && entry.get().krate != cnum {
            entry.insert(parent);
        }
    }
```

This causes items to be associated with the crates they were
originally defined in, even if we had encountered them during the
traversal of an earlier crate.

(Having said that, I am not clear on why this same logic does not
apply when both rust-lang#46708 and rust-lang#46838 have been applied. But at this
point, I am just happy to have a plausible explanation for why we see
`core::foo::bar` in the output for these tests, and want to focus on
getting this fix for rust-lang#46112 backported to beta.)
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

⌛️ Testing commit aa030dd with merge 5165ee9...

Contributor

bors commented Dec 22, 2017

⌛️ Testing commit aa030dd with merge 5165ee9...

bors added a commit that referenced this pull request Dec 22, 2017

Auto merge of #46838 - pnkfelix:issue-46112-followup, r=estebank
Followup for #46112.

Sorting by crate-num should ensure that we favor `std::foo::bar` over
`any_other_crate::foo::bar`.

Interestingly, *this* change had a much larger impact on our internal
test suite than PR #46708 (which was my original fix to #46112).
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 5165ee9 to master...

Contributor

bors commented Dec 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 5165ee9 to master...

@bors bors merged commit aa030dd into rust-lang:master Dec 22, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors added a commit that referenced this pull request Dec 23, 2017

Auto merge of #46905 - pnkfelix:backport-46112-fix-to-beta, r=michael…
…woerister

Backport 46112 fix to beta

Its probably easiest to focus on the diff rather than the commit series.

If you prefer I could squash them, but I figured preserving the cherry-picking will make it easier to relate what has happened here to what happens on the `master` branch.

Note: This is a backport of *just* #46838. It does not include #46708 (which we may end up wanting to revert).

Fix #46112
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 10, 2018

Member

Looks like we forgot to backport this for 1.23.0 (sorry about that!) so removing beta tags

Member

alexcrichton commented Jan 10, 2018

Looks like we forgot to backport this for 1.23.0 (sorry about that!) so removing beta tags

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