string iterator cleanup #2026
string iterator cleanup #2026
Conversation
the simplest possible solution; it just calls str::chars_iter to do the iteration
and use them in place of the few spots that used the str versions
Conflicts: src/comp/syntax/ext/qquote.rs src/libcore/core.rc src/libcore/iter.rs src/libcore/str.rs
…, otherwise don't attempt to manage the submodules
…ant. solves the ugliness with importing the correct iterator
|
Thanks db48x. After reading the first two commits, the most obvious thing about this is that it changes a bunch of by-value character arguments to by-reference arguments. This doesn't seem like a good trade off to me. Can we accomplish the same things without doing that? Please also try to keep the history clean. It's difficult to evaluate this with so many merges. |
|
So after reading through the entire series I saw that a lot of my comments were addressed in later commits. That's great. I think I like where this is going, but it's a little difficult to see the big picture. It would be really nice if the history were cleaner and there were less diversions and corrections along the way. This series does take a firm stance on the direction we're going with the iter library, so I would like to hear others' opinions about it. @nikomatsakis @marijnh |
|
As an aside, I've been working on putting together static extension methods for the most commonly used types, and if we go in this direction it will make some conveniences like |
|
Yes, I think the by_chars iterator should be the default for strings. Or perhaps by_lines, it's used more often at the moment. |
|
Next time, when updating a pull request rebase on top of master, then force-push to your branch, rather than adding merge commits. This has become quite hard to untangle. |
|
Okay, I'm very sorry, but I'm giving up. I don't understand the intents behind these patches well enough to merge them sanely, and to make sense of the result. A lot of code, as well as docstrings, has changed since these patches were committed (which arguably, is our fault for letting it sit so long). @db48x If you have the time, please reorganize your branch to A) apply cleanly on top of head, B) be free of merge commits, C) rebase out tiny fixing commits and churn, so that each commit serves a single, significant purpose. Also relevant here is that we will soon be getting rid of the ++/&& argument mode distinction, so changing the modes of functions all over the standard libs is not a very good use of your time. |
|
Indeed, it seems that there is a disconnect between our development styles. You guys expected a perfectly polished patch series; while I dislike rebasing so what you see is exactly what I did, in the order I did it. I should have mentioned it. Anyway, I'll see what I can do to fix it up. |
|
I took another shot at rebasing and squashing this into a patch to apply against master with no luck. Maybe it could be squashed against an earlier rev and then applied to master. |
|
That's kind of you, but it may not be necessary. I did it over the weekend but didn't push it because it may no longer make as much sense as before. I need to play with the new extensions interface some first. |
|
Regretfully, this will require significant rework to merge, so I am going to close it. @db48x I apologize about this and thank you for your contributions and effort. |
This is for issue #1729