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

impl Add<{str, Cow<str>}> for Cow<str> #36430

Merged
merged 1 commit into from Sep 30, 2016

Conversation

Projects
None yet
5 participants
@llogiq
Copy link
Contributor

llogiq commented Sep 12, 2016

cc #35837

@alexcrichton alexcrichton added the T-libs label Sep 13, 2016

@llogiq

This comment has been minimized.

Copy link
Contributor

llogiq commented Sep 13, 2016

r? @alexcrichton – or do we need to discuss this first?

if self == "" {
Cow::Borrowed(rhs)
} else if rhs == "" {
self.clone()

This comment has been minimized.

@alexcrichton

alexcrichton Sep 14, 2016

Member

This can just be self, right?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 14, 2016

Thanks for the PR! Could you expand on the rationale for this as well? E.g. it seems specifically for the optimization where one side is empty?

@llogiq

This comment has been minimized.

Copy link
Contributor

llogiq commented Sep 14, 2016

Exactly. I have found this optimization while looking at Raph Levien's xi-editor, and thought it'd be a good idea to have it in libcollections.

@AndrewBrinker

This comment has been minimized.

Copy link
Contributor

AndrewBrinker commented Sep 16, 2016

There's a relevant discussion happening in the internals forum right now: https://internals.rust-lang.org/t/implement-add-for-string-string/4088/7

Personally, I am opposed to adding further impls where Add is used for string concatenation. There is obviously already one impl for this use case, but I think this was a design mistake, and shouldn't be made worse. It would be preferable, if possible, to have some other operator for concatenation in general, and then implement said operator for all relevant types (I do not know how feasible such a change is at this point, it may be that it is not feasible at all). If nothing else, Add as implemented here is non-commutative, which goes against what I imagine is many people's intuition (certainly mine) for the addition operator.

As an addendum to that, it may be a good idea to lay out what basic mathematical operator properties we like for the operator trait implementations to respect (commutativity, associativity, etc.). Obviously, as they are safe traits, such properties couldn't be relied on in safe code, but including an explanation of them in the docs would (hopefully) encourage the creation of implementations which respect users' intuitions for those operators.

@llogiq

This comment has been minimized.

Copy link
Contributor

llogiq commented Sep 16, 2016

I know where you come from, but Add for String is pretty much entrenched by now (I even had to allow the clippy lints for it due to complaints). And I disagree that we'd be making things worse – if you're opposed to String addition, you can always extend. OTOH, improving Cow ergonomics is a win in my book.

@AndrewBrinker

This comment has been minimized.

Copy link
Contributor

AndrewBrinker commented Sep 16, 2016

Yeah, you may be right given that string concatenation via Add already exists in the language. There's no good reason to leave performance improvements on the table out of an ideological opposition to something that's already been stabilized. Removing the Add impls for string concatenation and replacing them with some alternative operator may be a thing to consider for Rust 2.0 (if/when that arrives).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 25, 2016

I've tagged for discussion in libs triage, but I agree that it's probably too late to remove Add on String and those types. It's already stable today so it'd be a bit odd to hold off on this purely because of that.

Also @llogiq can you add AddAssign implementation as well?

@llogiq

This comment has been minimized.

Copy link
Contributor

llogiq commented Sep 27, 2016

That took way longer than usual due to my notebook (which was broken by my kids) failing when it gets hot – for example during a Rust build.

@alexcrichton should I add anything else?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 29, 2016

Ah nope, just waiting on the libs team to discuss. Our conclusion was that this should be good to go. Want to squash the commits as well?

@llogiq

This comment has been minimized.

Copy link
Contributor

llogiq commented Sep 29, 2016

My son Gregor was born yesterday, so since then I'm in childrearing mode. Feel free to squash while merging. 😄

scratch that, the kid's sleeping, found the time to squash. 😀

impl {Add, AddAssign}<{str, Cow<str>}> for Cow<str>
This does not actually add anything that wasn't there, but is merely an
optimization for the given cases, which would have incurred additional
heap allocation for adding empty strings, and improving the ergonomics
of `Cow` with strings.

@llogiq llogiq force-pushed the llogiq:cow_add branch from b9b4810 to dd13a80 Sep 29, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 29, 2016

@bors: r+

Oh wow, congratulations! I can definitely take over from here :)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 29, 2016

📌 Commit dd13a80 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 29, 2016

⌛️ Testing commit dd13a80 with merge c717cfa...

bors added a commit that referenced this pull request Sep 29, 2016

Auto merge of #36430 - llogiq:cow_add, r=alexcrichton
impl Add<{str, Cow<str>}> for Cow<str>

cc #35837
@ollie27
Copy link
Contributor

ollie27 left a comment

It might be worth adding tests for Cow + &str as well.

@@ -270,3 +270,49 @@ impl<'a, T: ?Sized + ToOwned> AsRef<T> for Cow<'a, T> {
self
}
}

#[stable(feature = "cow_add", since = "1.13.0")]

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

These will need to be 1.14.0 now.

} else if rhs == "" {
self
} else {
Cow::Owned(self.into_owned() + rhs)

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

If self is a Cow::Borrowed it would be better to use String::with_capacity to create the new string so we avoid an extra reallocation.

impl<'a> AddAssign<&'a str> for Cow<'a, str> {
fn add_assign(&mut self, rhs: &'a str) {
if rhs == "" { return; }
self.to_mut().push_str(rhs);

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

This doesn't behave the same as the Add implementations. If self is empty this should result in a Cow::Borrowed like with Add not Cow::Owned.

@@ -0,0 +1,65 @@
// Copyright 2012-2013-2014 The Rust Project Developers. See the COPYRIGHT

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

You'll need to add mod cow_str; to src/libcollectionstest/lib.rs for this file to be used.

Also I'm pretty sure "2012-2013-2014" isn't the correct year 😉.

assert_eq!("Hello, Rustaceans!", borrowed1 + owned2);

assert_eq!("Hello, World!", owned1 + borrowed2);
assert_eq!("Hello, Rustaceans!", owned1 + owned2);

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

These two should presumably start with "Hi, ".

Also owned1 and owned2 have been consumed by this point so will need cloning or something earlier.

}
}

fn check_cow_add_assign() {

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

This will need #[test].

impl<'a> Add<&'a str> for Cow<'a, str> {
type Output = Cow<'a, str>;

fn add(self, rhs: &'a str) -> Self {

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

I think it would be worth adding documentation to these implementations as the semantics aren't really obvious. Especially explaining why the lifetimes have been restricted like they have.

type Output = Cow<'a, str>;

fn add(self, rhs: &'a str) -> Self {
if self == "" {

This comment has been minimized.

@ollie27

ollie27 Sep 29, 2016

Contributor

nit: might be better to use .is_empty().

@bors bors merged commit dd13a80 into rust-lang:master Sep 30, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@llogiq llogiq deleted the llogiq:cow_add branch Dec 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment