Remove unnecessary calls to Str::as_slice by richo · Pull Request #22561 · rust-lang/rust

richo · 2015-02-20T05:18:35Z

This may not be quite ready to go out, I fixed some docs but suspect I missed a bunch.

I also wound up fixing a bunch of redundant [] suffixes, but on closer inspection I don't believe that can land until after a snapshot.

rust-highfive · 2015-02-20T05:18:45Z

r? @gankro

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

richo · 2015-02-20T05:25:01Z

Ok, I think I got everything, but no doubt I missed something in a test, on a platform I don't have to test on. A bors run would be helpful.

there were cases where Deref coercions would have worked, but I opted for as_str anyway, as it made the surrounding code more readable (eg, the fact that it was going to coerce to a str was not obvious)

richo · 2015-02-20T07:08:42Z

Following up from IRC, the places where I left .as_str(), eg the assertions were all places where it seemed that as a reader, explicitly taking the underlying str wasn't obvious.

More than happy to go back and fix them all

Gankra · 2015-02-20T14:08:10Z

Places like these I think should be fine to drop the as_str from, clarity-wise.

Gankra · 2015-02-20T14:18:02Z

Yeah going over this the as_str's don't really seem to add anything outside of a generic context. I would just drop 'em where you can.

richo · 2015-02-20T16:37:05Z

Thanks for all the feedback! I'll revise when I get a chance.

alexcrichton · 2015-02-20T21:27:55Z

Could you also leave as_slice in-place but #[deprecated] for now? I suspect there is a lot of code to go clean up outside the repo still using .as_slice() :)

nikomatsakis · 2015-03-04T19:49:51Z

(Sadly, needs a rebase.)

richo · 2015-03-08T06:31:43Z

I redid this patch from scratch (I had some fairly lurky attempts to fix tests from the last time). Running make check now

richo · 2015-03-08T21:57:08Z

r? @gankro I believe this is good to go out. the lldb tests are failing locally for me, but I believe it's because I have a wedged lldb install

(Error in case it's not something I've done is Fatal Python error: PyThreadState_Get: no current thread which then aborts python)

richo · 2015-03-08T23:35:38Z

Aaaaaand all tests pass on linux \o/

alexcrichton · 2015-03-09T05:03:13Z

This could be replaced with &s[..]

There are a bunch of warnings about places I could use slice syntax, I figured I'd do this in two passes to make landing it feasible, although this was less churn than I expected.

I also wasn't sure if the syntax was dependant on a snapshot.

Yeah the slicing syntax should definitely be usable as-is today, thankfully it's been around for awhile!

alexcrichton · 2015-03-09T05:05:13Z

It's still a little unclear to me whether we want to do this or not. I think that if generic conversion traits land then there's no real need for the Str trait or the as_str method as they're basically just specializations of that trait.

I think it'd be totally fine, however, to land as much removal of as_str as possible for now though!

richo · 2015-03-09T05:17:41Z

Ok, I'll probably have a PR for that subset of this one in the next few days.

alexcrichton · 2015-03-09T05:26:45Z

Thanks @richo!

richo · 2015-03-09T05:27:28Z

Thanks to totally unforeseen organisation on my part, the rebase to only remove as_slice()s was totally trivial. r? @alexcrichton

alexcrichton · 2015-03-09T05:31:14Z

Nice! I updated the title and removed the Closes #xxxx in the title, otherwise looks great to me!

alexcrichton · 2015-03-09T05:31:21Z

@bors: r+ 5a930fd7ba381551a00f287c1506937ebb612d1f

richo · 2015-03-09T05:32:46Z

Great catch, cheers!

bors · 2015-03-09T10:36:16Z

⌛ Testing commit 5a930fd with merge 3f61b5d...

bors · 2015-03-09T12:07:04Z

💔 Test failed - auto-linux-64-x-android-t

Manishearth · 2015-03-09T12:27:14Z

/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/process.rs:833:32: 835:53 error: the trait `core::ops::Fn<(char,)>` is not implemented for the type `collections::string::String` [E0277]
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/process.rs:833                 assert!(output.contains(format!("{}={}",
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/process.rs:834                                                 *k,
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/process.rs:835

richo · 2015-03-09T14:54:57Z

Test fixed, r?

Manishearth · 2015-03-09T15:23:32Z

@bors: r+

bors · 2015-03-09T15:23:34Z

~~@bors r=Manishearth 7981aa6~~

richo · 2015-03-09T15:42:04Z

@Manishearth you might want to remove this from the rollup, I was just eyeballing it and spotted a few things that look off, I think it got mangled in the rebase. I have make check running here, but I don't want to toast another rollup :(

Manishearth · 2015-03-09T16:34:03Z

(already was removed 😄 )

richo · 2015-03-09T16:45:51Z

Ok, make check passes. I'm planning to hand hold this through bors, since I don't have access to an android test environment. (Fingers crossed I caught everything this time though)

bors · 2015-03-09T21:02:51Z

⌛ Testing commit 7981aa6 with merge b83b26b...

This may not be quite ready to go out, I fixed some docs but suspect I missed a bunch. I also wound up fixing a bunch of redundant `[]` suffixes, but on closer inspection I don't believe that can land until after a snapshot.

bors · 2015-03-09T23:27:12Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

rust-highfive assigned Gankra Feb 20, 2015

richo force-pushed the as_slice-as_str branch from 7539298 to 2095799 Compare February 20, 2015 05:24

richo force-pushed the as_slice-as_str branch 2 times, most recently from 7829834 to dc54f08 Compare February 20, 2015 05:35

Gankra reviewed Feb 20, 2015
View reviewed changes

richo force-pushed the as_slice-as_str branch 2 times, most recently from 4c81487 to 40e4262 Compare March 8, 2015 06:30

richo force-pushed the as_slice-as_str branch 2 times, most recently from 1f64c1f to f1357cd Compare March 8, 2015 20:27

alexcrichton reviewed Mar 9, 2015
View reviewed changes

richo force-pushed the as_slice-as_str branch from f1357cd to c94bf20 Compare March 9, 2015 05:25

richo force-pushed the as_slice-as_str branch from c94bf20 to 5a930fd Compare March 9, 2015 05:26

rust-highfive assigned alexcrichton and unassigned Gankra Mar 9, 2015

alexcrichton changed the title ~~str: rename as_slice to as_str~~ Remove unnecessary calls to Str::as_slice Mar 9, 2015

richo added 2 commits March 9, 2015 07:54

remove uses of as_slice where deref coercions can be used

061d843

doc: Fix extraneous as_slice()'s in docstrings

7981aa6

richo force-pushed the as_slice-as_str branch from 5a930fd to 7981aa6 Compare March 9, 2015 14:54

bors merged commit 7981aa6 into rust-lang:master Mar 9, 2015

bors mentioned this pull request Mar 9, 2015

Interpolate AST nodes in quasiquote. #23085

Merged

Uh oh!

Conversation

richo commented Feb 20, 2015

Uh oh!

rust-highfive commented Feb 20, 2015

Uh oh!

richo commented Feb 20, 2015

Uh oh!

richo commented Feb 20, 2015

Uh oh!

Gankra Feb 20, 2015

Choose a reason for hiding this comment

Uh oh!

Gankra commented Feb 20, 2015

Uh oh!

richo commented Feb 20, 2015

Uh oh!

alexcrichton commented Feb 20, 2015

Uh oh!

nikomatsakis commented Mar 4, 2015

Uh oh!

richo commented Mar 8, 2015

Uh oh!

richo commented Mar 8, 2015

Uh oh!

richo commented Mar 8, 2015

Uh oh!

alexcrichton Mar 9, 2015

Choose a reason for hiding this comment

Uh oh!

richo Mar 9, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichton Mar 9, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 9, 2015

Uh oh!

richo commented Mar 9, 2015

Uh oh!

alexcrichton commented Mar 9, 2015

Uh oh!

richo commented Mar 9, 2015

Uh oh!

alexcrichton commented Mar 9, 2015

Uh oh!

alexcrichton commented Mar 9, 2015

Uh oh!

richo commented Mar 9, 2015

Uh oh!

bors commented Mar 9, 2015

Uh oh!

bors commented Mar 9, 2015

Uh oh!

Manishearth commented Mar 9, 2015

Uh oh!

richo commented Mar 9, 2015

Uh oh!

Manishearth commented Mar 9, 2015

Uh oh!

bors commented Mar 9, 2015

Uh oh!

richo commented Mar 9, 2015

Uh oh!

Manishearth commented Mar 9, 2015

Uh oh!

richo commented Mar 9, 2015

Uh oh!

bors commented Mar 9, 2015

Uh oh!

bors commented Mar 9, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants