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 owned string literal #13548

Closed
wants to merge 1 commit into from

Conversation

richo
Copy link
Contributor

@richo richo commented Apr 16, 2014

Had a quick stab at this with a sed macro.

Fixed a lot of strings, but there's a bunch of tidy errors because of the increased line lengths now, definitely some broken tests, and isn't complete.

Pushing purely so that there's state on this. /cc #13543

@richo
Copy link
Contributor Author

richo commented Apr 16, 2014

So having played with this a little.. should I be using the str! macro instead, purely for brevity?

This upsets a LOT of checking because of how much longer it makes lines that include more than one string.

@brson
Copy link
Contributor

brson commented Apr 16, 2014

@richo No, I think what you've done with to_owned is fine. Can you rebase so this can be merged? This looks a good start, and I think it's best to merge in pieces so to avoid clashing with @pcwalton.

@richo
Copy link
Contributor Author

richo commented Apr 17, 2014

I broke a bunch of tests while I was doing this, I think (based on my make check) run last night. I think mostly to do with the repr of strings.

Regardless I'll rebase and push now.

@richo
Copy link
Contributor Author

richo commented Apr 17, 2014

(FWIW rustc builds and does reasonable things with my stripe bindings after this patch)

@richo
Copy link
Contributor Author

richo commented Apr 17, 2014

Rebased.

@brson
Copy link
Contributor

brson commented Apr 17, 2014

@richo GitHub still says it can't merge. Maybe something landed that conflicted after you rebased (perhaps another good reason to do this type of change in small chunks).

@richo
Copy link
Contributor Author

richo commented Apr 17, 2014

Entirely valid. Should I split this into per-file commits maybe?

@richo
Copy link
Contributor Author

richo commented Apr 17, 2014

I just rebased again. Assuming noone pushes anything conflicting before you get a chance to look at it, this should be merge-able, but I'm open to the idea of splitting this up a bit.

My only real concern is the amount of merge bubbles I'll create by doing this one lib at a time. Totall appreciate you working with me on this though :)

@richo richo changed the title [WIP] Remove owned string literal Remove owned string literal Apr 17, 2014
@richo
Copy link
Contributor Author

richo commented Apr 17, 2014

I'm getting this failure locally too:

/home/travis/build/mozilla/rust/src/libstd/tuple.rs:272:21: 272:35 error: type `&'static str` does not implement any method in scope named `to_owned`
/home/travis/build/mozilla/rust/src/libstd/tuple.rs:272         let a = (1, "2".to_owned());
                                                                            ^~~~~~~~~~~~~~

Is this a bootstrapping issue? eg, it's building a stage with a rustc too old to know about it?

@brson
Copy link
Contributor

brson commented Apr 17, 2014

@richo No, probably not. The to_owned fn is defined by the std::str::StrSlice trait, which is typically imported by the prelude, so available everywhere. std though does not import the prelude, so all imports must be explicit. This code is probably just missing the import.

@richo
Copy link
Contributor Author

richo commented Apr 17, 2014

Thanks, I was just about to push changes backing out of the change in those modules, but instead I'll fix it. I won't push again till the tests pass locally.

@richo
Copy link
Contributor Author

richo commented Apr 18, 2014

That unused import warning is a bug, since StrSlice is used in both a #[cfg(test)] and a #cfg(not(test))].

@richo
Copy link
Contributor Author

richo commented Apr 18, 2014

These test breakages are new to me (but consistent with my dev environment).

Any ideas @brson / @pcwalton ?

(Sorry for the delays in getting this in)

@@ -9,7 +9,8 @@
// except according to those terms.

fn foo(b: bool) -> Result<bool,~str> {
Err(~"bar"); //~ ERROR: cannot determine a type for this expression: unconstrained type
//~ ERROR: cannot determine a type for this expression: unconstrained type
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be written as:

Err(...);
//~^ ERROR: cannot determine a type for this expression: unconstrained type

The line with the error should go right next to the statement that should fail. If there's a tidy error, you can do what I've done in the example above. That is, put the expected error in the line below and use a ^ to specify that the expected error should be raised by the line above the comment.

@richo
Copy link
Contributor Author

richo commented Apr 18, 2014

Thanks for all the feedback! Updateing now.

@richo
Copy link
Contributor Author

richo commented Apr 18, 2014

I think the tests all pass locally (I'm waiting on make clean && make -j4 check now), and I've rebased on master.

@richo
Copy link
Contributor Author

richo commented Apr 18, 2014

And the tests pass!! \o/

/cc @brson

@brson
Copy link
Contributor

brson commented Apr 18, 2014

Since I never get to this before it goes stale, I've rebased and pushed to my own branch to land: #13607

It should land today. Thanks for your patience!

@brson brson closed this Apr 18, 2014
bors added a commit that referenced this pull request Apr 18, 2014
bors added a commit that referenced this pull request Apr 19, 2014
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2022
Fix `tt::Punct`'s spacing calculation

Fixes rust-lang#13499

We currently set a `tt::Punct`'s spacing to `Spacing::Joint` unless its next token is a trivia (i.e. whitespaces or comment). As I understand it, rustc only [sets `Spacing::Joint` if the next token is an operator](https://github.com/rust-lang/rust/blob/5b3e9090757da9a95b22f589fe39b6a4b5455b96/compiler/rustc_parse/src/lexer/tokentrees.rs#L77-L78) and we should follow it to guarantee the consistent behavior of proc macros.
Manishearth pushed a commit to Manishearth/rust that referenced this pull request Nov 23, 2022
internal: Update proc-macro-srv tests

Should have been included in rust-lang#13548, but I didn't notice as those tests aren't run in our CI.

cc rust-lang#104454
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.

3 participants