add copy constructor to byUTF()

[WalterBright]

A ForwardRange's save() function has a confusing relationship with the copy constructor. This helps clarify it. save() acts like a copy constructor, but it uses assignment semantics instead of construction semantics, carrying the seeds of bugs, besides making it impossible to call save() on a const object. This PR has save() use a constructor to construct the copy.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6900"

[WalterBright]

Continuing to call save() in the implementation will be necessary until copy constructors become de riguer.

[jmdavis]

@WalterBright

A ForwardRange's save() function has a confusing relationship with the copy constructor. This helps clarify it. save() acts like a copy constructor, but it uses assignment semantics instead of construction semantics, carrying the seeds of bugs, besides making it impossible to call save() on a const object. This PR has save() use a constructor to construct the copy.

Continuing to call save() in the implementation will be necessary until copy constructors become de riguer.

Wait, wait, wait. Are you trying to say that copy constructors are supposed to replace save now? That can only work if classes can't be ranges, because classes don't have copy constructors. They're reference types. As such, range-based code can't rely on copying ranges. You need to call save for that to work with all forward ranges. And in general, copying a range has undefined semantics. If it's a reference type, then you end up with another reference to the same thing. If it's a value type (including if it implements a postblit constructor or copy constructor to behave that way), then it's equivalent to calling save. And if it's pseudo-reference type, then what happens depends on what the range's members are. In the case of something like a dynamic array, that ends up being equivalent to calling save, but for structs, it could easily end up being the case that if you iterate the original after iterating the copy (or vice versa), the range will be screwed up, because the value types in the range were independent, but the reference types weren't. In fact, a wrapper range like byUTF risks that exact problem if it's a wrapping range which is a reference type, because it has a buffer which is a static array - and thus a value type. So, if you copy the wrapper range and then iterate either the original or the copy, then any time that popFront would just change the value types in the range to iterate to the next element, it would only affect the one being directly iterated, whereas any time that calling popFront pops the next element off of the wrapped reference type range, it will affect both.

So, you can't just copy a range and expect the behavior of save - far from it. Are you suggesting that we start requiring that forward ranges implement copy constructors in order to be a forward range rather than that they implement save? If we did that, then we could start relying on copying a forward range resulting in an independent copy, but that wouldn't work with classes - we'd have to start making it so that classes can't be forward ranges - and it would be problematic in the cases where a struct doesn't actually need a copy constructor in order to make it so that copying it results in an independent copy, because then you'd basically be forced to declare an unnecessary copy constructor just to make it so that isForwardRange treated it as a forward range.

[jmdavis]

Okay. I probably need to re-read the copy constructor DIP, but this looks like it's defined to only copy the range being wrapped and not the other members. If so, then how is this range being copied properly? Also, was it the intention to make it equivalent to calling save? If so, then it's not, because it doesn't call save on the range being wrapped, and what happens then depends entirely on how that particular range is implemented (e.g. if it's a class, it's not being saved).

[PetarKirov]

@jmdavis I think you got confused about what is happening here. This PR added two constructors, none of which are copy-constructors in the C++ sense (T(cv-qualifiers_opt T&):

1. The first one simply creates a new instance with blank state. It is used only at the end of the function: return Result(r);.
   It is not used for copying Result type, nor it is used in the save function.
2. The second constructor has all the necessary parameters to copy the whole internal state, so it was what Walter referred to as "copy constructor", even though it's not one in the strict C++ sense, or what is proposed by the DIP. This constructor is used inside the save() member function.

All in all, implementing save() for struct ranges is much more elegantly done via copy-construction (not only in the strict C++ sense), so net result of this PR is that the code is more straightforward and the compiler has easier time tracking scope/return.
These changes have nothing to do with the copy-constructor DIP, nor you should judge them as some grand language-breaking initiative. It's just that for this particular piece of code, this refactoring was the most beneficial.