Skip to content

Add .insert() and .insert_char() methods to ~str. #10826

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

Merged
merged 1 commit into from
Jan 6, 2014

Conversation

SimonSapin
Copy link
Contributor

I could not run the tests because of unrelated building issue, sorry about that.

@thestinger
Copy link
Contributor

One of the design decisions made by the C++ standard is to avoid exposing inefficient convenience methods like this. I would expect these on a rope type but I'm not really sure they make sense on a string.

There's also the issue of extending the usage of fairly meaningless byte indices.

@SimonSapin
Copy link
Contributor Author

str.unshift_char is already there, and just as inefficient. Sometimes O(N) behavior is really what one wants. Should every program just re-implement basic methods like this?

As to byte indices, this can be changed to what ever .slice() and related methods are changed to, when and if they are changed.

@thestinger
Copy link
Contributor

A program needing this should probably be using a rope though. The motivation behind that C++ design choice is to encourage using the right type, by making inefficient code inconvenient.

Inserting text into a Unicode code string is not trivial to do correctly, and we're not really helping by pretending it's as easy as giving a byte or code point index. A code point doesn't correspond to the semantic characters most programmers expect them to, and we're not helping them.

@SimonSapin
Copy link
Contributor Author

I thought I needed this for punycode decoding, but it turns out I really needed ~[char] which does have .insert().

I still disagree with your argument, though. Yes, proper text handling is hard. That doesn’t mean we should not provide any and leave everyone to implement it themselves, possibly even more wrong than what we could have provided.

In any case, if the answer is "use a rope instead" it would be nice to have a rope type in libextra. (Although that’s not really related to this bug.)

@ghost
Copy link

ghost commented Dec 5, 2013

str::slow::insert()?

@lbonn
Copy link
Contributor

lbonn commented Dec 5, 2013

@SimonSapin There used to be a rope type in extra (until #7629), if this can be of any help.

@thestinger
Copy link
Contributor

A rewrite is necessary for a correct, useful rope.

@SimonSapin
Copy link
Contributor Author

This only reinforce my point that "just use a rope" is not the answer to insert on ~str.

Anyway, I finished implementing punycode, using insert on ~[char]: https://github.com/SimonSapin/rust-url/blob/master/punycode.rs
I consider this good enough. I’m not worried about O(n²)-ish behavior since n is not bigger than 63 for domain name labels.

@brson
Copy link
Contributor

brson commented Dec 5, 2013

I don't particularly have a problem with this, though the docs could indicate the performance pitfalls as well as the behavior when the position isn't on a codepoint boundary. Any other opinions?

@thestinger
Copy link
Contributor

@SimonSapin: I don't understand why the poor state of the standard library would be an argument against ropes.

@SimonSapin
Copy link
Contributor Author

@brson Yes. I will update the pull request to do so. (Though this applies to the rest of the standard library as well.)

@thestinger I mean that #7629 is a sign that ropes are complex and not obvious to get right, and thus are not always a good answer to "I need to insert".

@thestinger
Copy link
Contributor

The effort required to implement a type like a hash table, rope or balanced binary search tree says nothing about their ease of use or suitability for a certain purpose. Vectors obviously aren't easy to get right either because Rust's vectors are full of overflow bugs and are slow.

@adrientetar
Copy link
Contributor

+1

@brson
Copy link
Contributor

brson commented Dec 10, 2013

I do notice that vecs insert method has a different signature: fn insert(&mut self, i: uint, x:T). If we do add a string insert method it should probably have the same signature as vec.

@SimonSapin
Copy link
Contributor Author

@brson Do you mean the order of the parameters? I can change that.

@brson
Copy link
Contributor

brson commented Dec 31, 2013

@SimonSapin yeah, the argument order is different for the two.

@SimonSapin
Copy link
Contributor Author

@brson Rebased, and fixed the argument order.

bors added a commit that referenced this pull request Jan 6, 2014
I could not run the tests because of unrelated building issue, sorry about that.
@bors bors merged commit 80d0f60 into rust-lang:master Jan 6, 2014
@SimonSapin SimonSapin deleted the str-insert branch January 9, 2014 14:35
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2023
Add lints for disallowing usage of `to_xx_bytes` and `from_xx_bytes`

Adds `host_endian_bytes`, `little_endian_bytes` and `big_endian_bytes`

Closes rust-lang#10765

v - not sure what to put here since this adds 3 lints
changelog: Add `host_endian_bytes`, `little_endian_bytes` and `big_endian_bytes` lints
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