Generalize sliced streams

[DanRStevens]

This lays some of the foundation for generalizing stream slicing. The FileSliceReader class doesn't really depend much on the specifics of the FileReader class. The class could eventually be refactored into a template that handles slices of arbitrary stream types.

This pull request aims to reduce some of the dependence of FileSliceReader on the specifics of FileReader. Most such dependencies were not particularly fundamental, but rather more in the wording of exception messages and names of variables and types.

This addresses some of the aspects brought up in Issue #72.

[DanRStevens]

I would be curious to get some feedback on this section of code as it pertains to the call from the copy constructor: https://github.com/OutpostUniverse/OP2Utility/blob/2282c34174a377a9cdc416fd56cb5d87e8a28b49/src/Stream/FileSliceReader.cpp#L23-L40

When copy constructing a slice object from another slice, these checks will have already run in the primary constructor. Does it make sense to run them again from the copy constructor for the copied object?

In the case of the first check, all values are const (startingOffset, sliceLength, numeric_limit::max()), so I'd say no there. As such, I've moved that check out to the primary constructor.

In the second check, the wrappedStream.Length() value is not const. It's possible for a stream to change length at runtime. In particular, the wrapped stream may shrink such that the slice of it now extends beyond the original wrapped stream. Considering this, it may make sense to check the length again when making a copy of a slice. However, if the copy is considered no good because the slice extends beyond the bounds of the wrapped stream, that means the original slice also now extends beyond the bounds of the wrapped stream. There is no way to declare the original source slice as invalid, or have it throw in any sort of way. My question then is, does it make sense to throw and prevent the copy construction?

Perhaps a deeper question, knowing that streams may change length, is the original check even valid? I think I would like to keep the original check. It does prevent unexpected behaviors, such as when trying to take too big of a slice of a fixed length stream, or trying to read from a partially written file. Though once the slice is created, it doesn't seem worthwhile to try and ensure the underlying stream doesn't shrink beyond the slice. I'm tempted to say, once a slice has been created, it may be reasonable to assume that slice and any sub-slice of that slice remains valid, even if the wrapped stream shrinks.

[DanRStevens]

Ok, I think I've hit a good stopping point for today.

The design is still a little rigid. In particular, the GetFilename method is still a little too specific for the template. That might be found on a few different streams, but not every stream. Perhaps that method can be removed, or only conditionally provided if the wrapped stream also provides it. Similarly, the IdentifySource() method used in exception messages could be specialized to provide info on other stream types, or as a default, just return an empty string if there is no specific info it knows how to return.

Another point is using BidirectionalSeekableReader as the base class. Not all streams will derive from this class. With a bit of work, and some template guards, it may be possible to derive from the same base class as the wrapped stream, and also only provide the appropriate method overrides.

That may be work for a separate branch though.

[Brett208]

I read through the commits. It all seems like positive changes to me. I'll carve out time later to download and play with the code and review in more depth.

[DanRStevens]

Hmm, I probably should have put more thought into this earlier before asking you to rename. Instead of ReaderSlice, maybe we should have used the name SliceReader. I was just looking at the other class names, and I think SliceReader would be more consistent.

Another point of clarification, I think we renamed the wrong thing. The template is currently named StreamSlice, and I think that construct should get the name SliceReader:

    template<class WrappedStreamType>
    class SliceReader : public BidirectionalSeekableReader

Meanwhile, the specific instantiation of that template, which wraps a FileReader should probably still be named FileSliceReader.

    using FileSliceReader = SliceReader<FileReader>;

The name of the header should probably be named after the template, hence SliceReader.h.

---

The specific instance using FileSliceReader = SliceReader<FileReader>; doesn't really need to be in the template's header file. I just put it there for backwards compatibility reasons during the transition period. It could get it's own header file, or it could just be declared where it is used, namely in FileReader.h, where there is already a forward declare for it. Actually, I think that forward declaration is sufficient. We should probably just remove the using FileSliceReader = ... line from the template header file.

---

Sounds like we should just largely ignore the stream size concern. You're right in that stream sizes can always change outside the control of our API. Namely, the filesystem API can change file sizes which we have readers for. We just have to accept the possibility of this happening.

I say let's just stick with what we have for now. What we have now fits our needs. Even if stream resizing was a bigger concern, I don't think it's a show stopper for the current API.

[Brett208]

Okay, no problem with the renames. I can try to fix this hopefully today or tomorrow. I probably didn't put enough time into thinking through the template verses the class.

-Brett