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

Next round of removing feature flags from libsyntax #24715

Closed
wants to merge 4 commits into from

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Apr 23, 2015

This is part two of #24518, which is trying to get libsyntax to compile without feature flags. After this, all that remains is #[feature(collections, libc, rustc_private, staged_api)]. The one real unfortunate thing is that I had to copy some tables from librustc_unicode, but I couldn't figure out a great way to share these tables between the two modules.

@rust-highfive
Copy link
Collaborator

r? @nrc

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

@erickt
Copy link
Contributor Author

erickt commented Apr 23, 2015

r? @alexcrichton

@nrc nrc assigned alexcrichton and unassigned nrc Apr 23, 2015
@@ -86,7 +86,31 @@ impl<'a> Iterator for LinkedPath<'a> {
}

/// The type of the iterator used by with_path.
pub type PathElems<'a, 'b> = iter::Chain<iter::Cloned<slice::Iter<'a, PathElem>>, LinkedPath<'b>>;
#[derive(Clone)]
pub struct PathElems<'a, 'b> {
Copy link
Member

Choose a reason for hiding this comment

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

Oh dear the instability of Cloned is actually a mistake and it's intended to be #[stable] (in which case this change may not be required)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see it's now stable in master, so I'll remove this patch :)

erickt added 4 commits April 26, 2015 17:18
…drop

While this is slower than before, it's not necessarily forward
compatible with planned changes to the drop flag. Benchmarking suggests
that this does not dramatically impact the compile times of rust.
@erickt
Copy link
Contributor Author

erickt commented Apr 27, 2015

@alexcrichton: Updated to remove the patches. I think I could even remove the P::map patch and just apply that to syntex if you think the performance benefit is worthwhile in rustc.

@erickt
Copy link
Contributor Author

erickt commented Apr 27, 2015

@kwantam: Moving the comments off the outdated commit. Yes, I was hoping that we could do what we did for char::width() and make it deprecated and point at this unicode-xid crate. By the way, I don't really know much about unicode, so if there's a more appropriate name for this then I'd be happy to change it. I originally had it as unicode-derived-property, but it turns out I think we have all the other derived properties exposed as stable char methods.

On making rustc dependent on crates.io, I haven't heard of any plans going in that direction. I do think though that it's a little dangerous that we have these internal and external crates diverging from one another. We should probably shift over to using git submodules to pull those packages in to rust-proper.

@kwantam
Copy link
Contributor

kwantam commented Apr 27, 2015

@erickt Regarding crates.io: I agree, it seems to make the bootstrapping problem a little gross.

crates.io appears down at the moment, but when it comes back I'll I've just pushed unicode-xid.

@SimonSapin
Copy link
Contributor

By the way, I don't really know much about unicode, so if there's a more appropriate name for this then I'd be happy to change it.

For what it’s worth, the relevant part of the standard is Unicode Standard Annex #31:
Unicode Identifier and Pattern Syntax
.

@SimonSapin
Copy link
Contributor

@erickt By the way, I saw a couple times a ping from you on IRC in what’s the middle of the night in my time zone, and by the time I saw it you were not on IRC anymore. Please remember http://www.nohello.com/ :)


// FIXME: This was copied from core/str/mod.rs because it is currently unstable.
#[inline]
pub fn slice_shift_char(s: &str) -> Option<(char, &str)> {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this function is actually pretty rarely used (only used twice here), could this just be removed entirely and each call site just have a custom implementation? They don't really need slice_shift_char in its full glory but rather just some smaller specialized versions.

@alexcrichton
Copy link
Member

Thanks @erickt! I'm fine with transitioning away from read_and_drop for now

@erickt
Copy link
Contributor Author

erickt commented Apr 27, 2015

@SimonSapin: @kwantam helped me out. I wasn't sure which of you two were in charge of unicode-rs, so I pinged you both :) I'm good now though, but thanks for checking in!

@alexcrichton: I'll look into simplifying slice_shift_char.

@bors
Copy link
Contributor

bors commented Apr 30, 2015

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

@alexcrichton
Copy link
Member

ping @erickt, is this still necessary?

@erickt
Copy link
Contributor Author

erickt commented Jun 9, 2015

@alexcrichton: nope!

@erickt erickt closed this Jun 9, 2015
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.

7 participants