Generalize sliced streams #249
Conversation
The more direct approach should be easier to read and understand. It also generalizes to both signed and unsigned types. The old code would be undefined, according to the C++ standard, if it had been used on signed types (though would have still likely worked correctly in most environments).
This separates some of the type specific wording in the exception message, and uses formatting that is easier to update.
The `FileReader` class already has a copy constructor, so use that instead of dealing with the specifics of constructing with a filename. Note: The `WrappedStreamType` alias can eventually become a template parameter.
This makes the phrasing a little more general, and reporting of wrapped stream specific data (filename) a little more consistent.
The old code, with recent changes, was creating a temporary copy of the `FileReader` using `GetFilename`. This copy was then passed to the `FileSliceReader` constructor, where it was copied again. This makes things a little more direct, with only a single copy.
The `GetFilename()` method is specific to file related streams. If slicing is generalized, and a different stream type is wrapped, this won't work. By extracting this now, it minimizes the number of places code will need to be modified to support wrapping a different stream type.
Note: The `Position()` method returned: `wrappedStream.Position() - startingOffset`
Slice parameter values should be in the domain of the slice. Currently some of them look to be in the domain of the wrapped stream. This makes the checks and usage of those values inconsistent. This test should help catch such errors.
The single parameter version of `Slice()` should pass an offset in the domain of the slice to the two parameter version of `Slice()`. This test should catch errors translating the single parameter calls to two parameter calls.
The `Slice()` methods should use paraemters in the domain of the slice, rather than the domain of the wrapped stream.
When copy constructing from an existing object, this bounds check will have already run on the original source object. Hence there is no reason to do it again in the copy. Side note: The `startingOffset` and `sliceLength` are both `const`.
|
I would be curious to get some feedback on this section of code as it pertains to the call from the copy constructor: OP2Utility/src/Stream/FileSliceReader.cpp Lines 23 to 40 in 2282c34 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 In the second check, the 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. |
To convert this to a template, all methods must be inline in the header.
|
Ok, I think I've hit a good stopping point for today. The design is still a little rigid. In particular, the Another point is using That may be work for a separate branch though. |
|
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. |
There was a problem hiding this comment.
Just a name change suggestion for now. If we perform the name change, we will want to update the name of the test file to match.
I pushed a PR for OP2Archive to update to the most recent version of OP2Utility. It had all the namespace changes in it so is riddled with small changes. Once that is settled, I'll use OP2Archive to do some more interesting checks on this.
It compiled fine without warnings on my CPU in x86 debug mode.
-Brett
src/Stream/FileSliceReader.h
Outdated
| // Access is bounds checked according to the slice parameters. | ||
| // The underlying stream must be copy constructible, so that an independent stream can be created. | ||
| template<class WrappedStreamType> | ||
| class StreamSlice : public BidirectionalSeekableReader |
There was a problem hiding this comment.
What do you think of renaming this to Stream::ReaderSlice? To me, Stream::StreamSlice would indicate a read and write capability (or just be vague on what it does?).
If this sounds good, I can update the filename and the Visual Studio project files.
There was a problem hiding this comment.
The name probably does need some work. I was purposefully vague on the name since I haven't really decided if this will apply only to read streams, or if it will generalize to write streams as well. I suppose I could just have two templates, though there would be a lot of code in common.
In terms of supporting different levels of the stream interface (Reader, Seekable, BidirectionalSeekable), there may need to be a lot of conditional code added to the template. As such, it may be a simple matter to conditionalize Read/Write, and have the template work for both stream types.
With that said, you're welcome to rename things. We can always rename again later if we come up with a better name.
There was a problem hiding this comment.
Sounds good. I'll rename to reflect read only for now and we can rename again if we ever progress.
Also rename associated unit test file
There was a problem hiding this comment.
Just ran OP2Archive using the new Stream::ReaderSlice. OP2Archive successfully constructed the ReaderSlice when extracting the contents of art.vol. The contents extracted properly both when OP2Archive was compiled in x86 and x64 mode. Between this test and the unit tests, I think we are safe to merge.
A few comments on the questions brought up by @DanRStevens
Considering bounds checking read streams: While it is true that the stream size being read could change, I feel like for most of my use cases, I expect the memory or file stream to be a static source when reading. So I would consider it helpful to keep bounds checking. This way I know if I made a mistake on the slice size or if the source is changing size unexpectedly.
If we wanted to get more sophisticated, perhaps there could be 2 versions, a Stream::ForwardReaderStatic and a Stream::ForwardReaderDynamic. This would cause a lot of branching though because I guess then you would have a Stream::BiDirectionalSeekableReaderStatic, etc.
Maybe we could add a flag to the seekable classes indicating if the reader should be static or not? This would cause the branching to occur inside the class and reduce the number of files and classes.
Either of these approaches might be misleading though because we are only bounds checking. Nothing is preventing someone from going inside a manually changing a WORD inside the stream contents without changing the length?
I don't have a strong preference either way. Sorry if that isn't helpful...
|
Hmm, I probably should have put more thought into this earlier before asking you to rename. Instead of Another point of clarification, I think we renamed the wrong thing. The template is currently named template<class WrappedStreamType>
class SliceReader : public BidirectionalSeekableReaderMeanwhile, the specific instantiation of that template, which wraps a using FileSliceReader = SliceReader<FileReader>;The name of the header should probably be named after the template, hence The specific instance 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. |
|
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 |
This lays some of the foundation for generalizing stream slicing. The
FileSliceReaderclass doesn't really depend much on the specifics of theFileReaderclass. 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
FileSliceReaderon the specifics ofFileReader. 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.