Cleaned up str module a bit #9392
Conversation
| @@ -268,7 +271,7 @@ impl<'self, S: Str> StrVector for &'self [S] { | |||
| result | |||
| } | |||
|
|
|||
| /// Concatenate a vector of strings, placing a given separator between each. | |||
| #[inline] | |||
There was a problem hiding this comment.
Do these really need to get tagged as inline? It seems like performance-critical code wouldn't be calling these functions anyway, and they're pretty large to get inlined.
There was a problem hiding this comment.
It was my understanding that an #[inline] hint merely allows llvm to cross-crate inline a function if it deems it useful.
As far as I know, this doesn't actually force an inline? Or is the growing size of the metadata section an problem?
There was a problem hiding this comment.
It's both growing the size of the metadata and slowing compilation of modules linking to libstd in the sense that they have more code to process with LLVM. There's really no need for these to get inlined across crates because they're fairly large chunks of code and probably wouldn't get inlined in the first place anyway.
There was a problem hiding this comment.
If you use #[inline], it no longer outputs a symbol in the library and forces it to be recompiled in each crate. It also puts a very strong inline hint on the function, overriding the regular cost heuristic. If you made a very bad choice, LLVM can choose to not inline it, but it will still have a pointless duplicate version of it per-crate.
|
All of the documentation movements look good to me, although I'm less certain about the renamings. I don't feel too strong either way, but there's should probably be more agreement. |
|
I removed most of the new the |
|
Cool, thanks! Let's get a few more opinions about the renamings before merging though. |
|
FWIW, a pile of languages use I'm not particularly fussed either way. Some other points: |
|
@cmr Rebased on master, all set for a review. :) |
| @@ -990,9 +991,14 @@ pub fn utf8_char_width(b: u8) -> uint { | |||
| return UTF8_CHAR_WIDTH[b] as uint; | |||
| } | |||
|
|
|||
| #[allow(missing_doc)] | |||
| /// Struct that contains a `char` and an next byte index. | |||
There was a problem hiding this comment.
"the index of first byte of the next char in a string."?
Moved OwnedStr doc comments from impl to trait. Added a few #[inline] hints. The doc comment changes make the source a bit harder to read, as documentation and implementation no longer live right next to each other. But this way they at least appear in the docs.
Moved `StrSlice` doc comments from impl to trait. Moved `OwnedStr` doc comments from impl to trait. Normalized a few `StrSlice` method names from `FOO_reverse`, `FOO_rev` and `rFOO` to `FOO_rev`. Added a few `#[inline]` hints. The doc comment changes make the source a bit harder to read, as documentation and implementation no longer live right next to each other. But this way they at least appear in the docs.
Moved
StrSlicedoc comments from impl to trait.Moved
OwnedStrdoc comments from impl to trait.Normalized a few
StrSlicemethod names fromFOO_reverse,FOO_revandrFOOtoFOO_rev.Added a few
#[inline]hints.The doc comment changes make the source a bit harder to read, as documentation and implementation no longer live right next to each other. But this way they at least appear in the docs.