Fix Issue 9456 - decodeFront is inconsistent in whether it pops elements off of the range or not #1129
Conversation
| static if (isRandomAccessRange!S && hasSlicing!S && hasLength!S) | ||
| str = str[numCodeUnits .. str.length]; | ||
|
|
||
| return retval; |
There was a problem hiding this comment.
Minor nitpicks here (though I realize 1 & 2 where prre-existing):
- Why does
canIndexcheck forisSomeChar!(ElementType!S))? - What is the point of
immutable fst = str[0];vsimmutable fst = str.front; - The last static if should re-use the definition of
canIndex. As it stands, both are slightly different, and I can't tell if this is intended.
There was a problem hiding this comment.
As far as I can tell, canIndex shouldn't need to check for isSomeChar!(ElementType!S). It's probably left over from a previous refactoring. The difference between str[0] and str.front stems from an avoidance of front with strings, but since this is the overload which doesn't use strings, it shouldn't matter here (probably missed in a previous refactoring as well). The condition in the second static if is fine and is really what the first one should be. Functionally, this code is fine, but it does look like it could use to be cleaned up a bit more due to what you've pointed out.
There was a problem hiding this comment.
Okay. That bit of code has been cleaned up.
|
I reviewed the new code, and I have nothing to add (apart from the above mentioned nitpick). I didn't look too much into the unittests. |
This change makes it so that decodeFront takes its range by ref and pops off the code units that it decodes (unlike decode). It pretty much has to do that, since it supports input ranges, and they can't do anything else.
This makes no semantic difference to the code, but it makes it so that the attributes which are on one of the overloads don't make it into the documentation (where they would imply that all of the overloads have those attributes, which is not correct).
|
Any more comments on this? |
Fix Issue 9456 - decodeFront is inconsistent in whether it pops elements off of the range or not
decodeFrontwas popping off elements from reference type ranges and input ranges but not other types. Ideally, it would act likedecodeand not pop any elements off of any range, but that doesn't work with input ranges. So, in order to keep it consistent,decodeFrontnow takes its range byrefand always pops off the code units that it decodes.Also, the tests have been improved for
decode,decodeFront,stride, andstrideBackso that they test more range types (including reference types ranges).