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 crate_visibility_modifier feature #1143

Closed
wants to merge 4 commits into from
Closed

Remove crate_visibility_modifier feature #1143

wants to merge 4 commits into from

Conversation

jhpratt
Copy link
Contributor

@jhpratt jhpratt commented Sep 20, 2019

In some situations, pub(crate) wasn't strictly necessary, as it was
within a context where it would not be exposed regardless of scope. In
these situations, pub is used per request.

Pinging @SergioBenitez and @jebrosen for review. This should be looked through carefully to ensure nothing is inadvertently exposed or hidden. Checking the documentation should be sufficient for this.

In some situations, `pub(crate)` wasn't strictly necessary, as it was
within a context where it would not be exposed regardless of scope. In
these situations, `pub` is used.
@jebrosen jebrosen self-requested a review September 20, 2019 20:52
Copy link
Collaborator

@jebrosen jebrosen left a comment

Choose a reason for hiding this comment

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

I'll do a more thorough review against a diff, but generally LGTM.

core/codegen/src/lib.rs Show resolved Hide resolved
@jhpratt
Copy link
Contributor Author

jhpratt commented Sep 21, 2019

For reproducing my check, you'll want to generate docs for the current master and this PR. Move the target/doc directories to some other (temporary) folder; I'll call these master and pr.

In both of these directories, run rm *.* and rm -r src. This will remove any artefacts that are unrelated to the diff and the file for source code — we know that one will change, we're only interested in the doc changes.

Next, run diff -r main pr > diff.txt. Open diff.txt up in a text editor. Since we're looking for the addition or deletion of functions, modules, constants, etc. the only lines relevant here are those with a difference in length. To quickly check this, I removed all irrelevant lines (---, 0c0, etc.), leaving just the file names and diff.

As of now, there are no lines with differing lengths, indicating that nothing was exposed or hidden accidentally.

Copy link
Collaborator

@jebrosen jebrosen left a comment

Choose a reason for hiding this comment

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

Everything looks to be either crate -> pub(crate) or a crate -> pub that I have checked by hand to be contained in a non-pub path.

core/codegen/src/lib.rs Outdated Show resolved Hide resolved
@jebrosen
Copy link
Collaborator

Test failures look unrelated -- UI test regression possibly from rust-lang/rust#64498.

@SergioBenitez
Copy link
Member

@jebrosen That looks like a serious regression. I'm not sure if that's the PR to blame, but we should figure out what is.

@jebrosen
Copy link
Collaborator

you'll want to generate docs for the current master and this PR

You would actually need to do this with all features enabled in both debug and release mode. But that difference only applies to one or two types, and I also manually checked that everything is okay.

I'll merge once my local test run finishes.

@jebrosen jebrosen self-assigned this Sep 21, 2019
@jebrosen
Copy link
Collaborator

Awesome! Merged as 4e6a7dd.

@jebrosen jebrosen closed this Sep 21, 2019
@jebrosen jebrosen added the pr: merged This pull request was merged manually. label Sep 21, 2019
@jhpratt jhpratt deleted the remove-crate-vis branch December 10, 2019 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: merged This pull request was merged manually.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants