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

save-analysis: emit names of items that a glob import actually imports. #19254

Merged
merged 3 commits into from
Dec 27, 2014

Conversation

nrc
Copy link
Member

@nrc nrc commented Nov 23, 2014

There is also some work here to make resolve a bit more stable - it no longer overwrites a specific import with a glob import.

r?

@nrc
Copy link
Member Author

nrc commented Nov 23, 2014

This is useful in itself for DXR-like tools, but it is also a proof of concept for a deglobber tool, which would convert glob imports to proper imports.

@huonw
Copy link
Member

huonw commented Nov 28, 2014

There has previously been debate and rejected PRs adding Show for RefCell, it is unusual because it introduces an implicit panic in otherwise sensible code. It would probably better to get some more feedback before adding it again. #14811, #14883, #16714.

Similarly, for Weak. #14759

#[experimental = "Show is experimental."]
impl<T: fmt::Show> fmt::Show for Weak<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
if self._ptr.is_null() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right condition to check. The pointer is always valid, and an invalid Weak is indicated by the strong count being 0.

@nrc
Copy link
Member Author

nrc commented Nov 29, 2014

@huon yeah, I realised I should have got review for that extra patch. I only included it because I needed it for debugging. I'll pull it out into a separate PR

@nrc
Copy link
Member Author

nrc commented Nov 29, 2014

In order to not break tests I had to change quite a lot of this patch. I also realised there is a breaking change here (I think covered by RFC 116).

I think there are enough changes that this needs a re-review, so r? @pcwalton again, please

@@ -396,6 +401,13 @@ impl Rib {
}
}

#[deriving(Show,PartialEq,Clone)]
enum Shadowable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document what this does in a comment?

@nrc nrc force-pushed the dxr-glob branch 2 times, most recently from c08775f to 4a407b9 Compare December 5, 2014 17:27
@nrc
Copy link
Member Author

nrc commented Dec 5, 2014

added comment, r? @pcwalton

@nrc nrc force-pushed the dxr-glob branch 2 times, most recently from ed7ab99 to 9d7e2e5 Compare December 11, 2014 04:43
@nrc
Copy link
Member Author

nrc commented Dec 11, 2014

ping for re-r? @pcwalton

@@ -341,6 +341,10 @@ pub fn phase_3_run_analysis_passes<'tcx>(sess: Session,
let lang_items = time(time_passes, "language item collection", (), |_|
middle::lang_items::collect_language_items(krate, &sess));

let make_glob_map = match save_analysis(&sess) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use "if" here

@pcwalton
Copy link
Contributor

r=me with one nit

bors added a commit that referenced this pull request Dec 18, 2014
There is also some work here to make resolve a bit more stable - it no longer overwrites a specific import with a glob import.

r?
nrc added 3 commits December 27, 2014 09:55
There is also some work here to make resolve a bit more stable - it no longer overwrites a specific import with a glob import.

[breaking-change]

Import shadowing of single/list imports by globs is now forbidden. An interesting case is where a glob import imports a re-export (`pub use`) of a single import. This still counts as a single import for the purposes of shadowing .You can usually fix any bustage by re-ordering such imports. A single import may still shadow (override) a glob import or the prelude.
bors added a commit that referenced this pull request Dec 27, 2014
There is also some work here to make resolve a bit more stable - it no longer overwrites a specific import with a glob import.

r?
@bors bors merged commit 4b92a5a into rust-lang:master Dec 27, 2014
bors added a commit that referenced this pull request Jan 1, 2015
r? @huonw or @alexcrichton

Apparently, we have previously rejected an RFC like this. However, since then we removed `{:?}` and so without this debugging gets really difficult as soon as there is a RefCell anywhere, so I believe there is more benefit to adding these impls than there was before. By using "try_borrow" we can avoid panicing in `Show` (I think).

@ huon in response to a comment in #19254: I noticed that `drop()` checks for the ptr being null, so I checked here too. Now I am checking for both, if you're confident I can change to only checking `strong()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants