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

std: Bring back half of Add on String #14482

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

This adds an implementation of Add for String where the rhs is <S: Str>. The
other half of adding strings is where the lhs is <S: Str>, but coherence and
the libcore separation currently prevent that.

@alexcrichton
Copy link
Member Author

This has been a highly requested feature since the move away from ~str. I agree that it is not performant, but the precedence is quite overwhelming.

@huonw
Copy link
Member

huonw commented May 28, 2014

Copying my comment from #14481 for posterity:


I'm not keen on the Add impl for string. It seems to easily encourage inefficient/overallocating code since it doesn't reuse the allocation of its argument, meaning s = s + a is horrible (especially in a loop). (And I relatively often see code approximately equivalent to that.)

I would be very interested in waiting a little longer to see if not having it is just too unbearable.

(Also this addition on strings isn't even commutative, i.e. it flies in the face of mathematical convention for the + operator. :P )

@huonw
Copy link
Member

huonw commented May 28, 2014

Travis failed btw.

@adrientetar
Copy link
Contributor

I agree. This seems too much like Python.

@kud1ing
Copy link

kud1ing commented May 28, 2014

I think people expect something like this (even when imperformant), since many other languages provide it: http://rosettacode.org/wiki/String_concatenation

@japaric
Copy link
Member

japaric commented May 28, 2014

How about we implement the AddAssign trait (when available) instead of the Add trait.

That way one can do s += a, that desugars to s.add_assign(&a), which is implemented as s.push_str(a.as_slice()) or s.push_char(a) depending on the type of a.

That way people can have some sugar, without us encouraging inefficient reallocations.

@bill-myers
Copy link
Contributor

How about adding new versions of add that take parameters by value instead of by reference (for a total of 4 versions), and then having the compiler automatically pass by value when the value is no longer used?

The more general s = any_function(s + a) would then result in a call to a variant of add that takes the first parameter by value , which would of course be implemented by appending in place (if there is space).

One could also automatically rewrite s = s + a to s += a so that it works when s is an lvalue dereference of a &mut.

@lilyball
Copy link
Contributor

@bill-myers That's rather magical behavior. Adding a later use of a variable should not silently change the semantics of earlier code. Note here that it's not just allocation semantics; the actual behavior could be subject to change as well, as this is just invoking user-defined code.

@huonw
Copy link
Member

huonw commented May 29, 2014

Theoretically we could actually just have Add take everything by-value and implement it like

impl<'a,'b> Add<&'b BigInt, BigInt> for &'a BigInt {
    fn add(self, other: &'b BigInt) -> BigInt { ... }
}

However, (with current-Rust) this would mean that a + b desugaring to just a.add(b) would require it to be called like &bigint + &bigint for types that take by reference (but all the primitives would be fine with by-value and so they would not require references).

fn add(&self, other: &S) -> String {
let mut s = self.to_string();
s.push_str(other.as_slice());
return s;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return s; -> s :)

This adds an implementation of Add for String where the rhs is <S: Str>. The
other half of adding strings is where the lhs is <S: Str>, but coherence and
the libcore separation currently prevent that.
@alexcrichton alexcrichton deleted the strbuf-add branch June 25, 2014 04:16
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
fix: Fix relative path creation using the wrong path accessor

This should hopefully fix the other errors with paths people seem to be getting
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.

None yet

9 participants