OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Allow reading ArtFile from rvalue streams #284

Closed DanRStevens closed 5 years ago

DanRStevens commented 5 years ago

Here's an idea I've been contemplating using more widely in this project. Being able to load data from temporarily constructed unnamed stream objects.

// Read from stream as lvalue (2 line construct)
auto reader = Stream::FileReader("filename.art"); // First create a named variable
auto artFile = ArtFile::Read(reader); // Pass the variable

// Read from stream as rvalue (unnamed temporary object) (1 line construct)
auto artFile = ArtFile::Read(Stream::FileReader("filename.art")); // Pass an unnamed object constructed inline

This is supported by an rvalue reference overload. Conveniently, such an overload can just delegate to the lvalue reference overload.

static ArtFile Read(Stream::Reader& reader);  // lvalue reference
static ArtFile Read(Stream::Reader&& reader); // rvalue reference

The first lvalue reference overload is needed when passing named values. It will not bind to a call with an unnamed temporary object.

The second rvalue reference overload is needed when passing an unnamed temporary object, such as one that is newly constructed inline, or returned from a function. It will not bind to a call with a named variable.


Aside: In some cases, a const reference can be used to bind to both lvalues and rvalues, however, this is only possible with const. Example:

static ArtFile Read(const Stream::Reader& reader); // lvalue reference or rvalue reference

Unfortunately this is completely useless for our purposes here, since a const stream would not be able to update its internal stream pointer, and hence would not be able to read (or write) data. In particular, there would be a compile error on calls to Read, which is a non-const function, and so can not be called on a const object.


Longer Aside: It is theoretically possible to bind lvalue reference methods to calls with rvalue references, and indeed MSVC had a non-standard extension that allowed it. However, this is considered potentially undesirable, and so the overload has to be explicitly specified in standards conforming code. The reason being, although an rvalue object is perfectly writable (it is temporary after all), access to those changes would be lost, since there is no name to refer to it afterwards. Hence code that writes to a temporary, which is then lost was deemed quite potentially an unintended bug. This is like how the compiler warns when you have an expression statement with no side effects.

5 + 7;  // Okay...?

In our case it makes sense to read from a temporary, as we'd likely just dispose of the stream after reading from it. The only thing being written was the stream position, which is irrelevant once we are done with the stream.

This can also be true of writing, such as when writing to a named file, where the written data can later be accessed using the filename to refer to it.

// Assuming an rvalue overload for writing
artFile.Write(Stream::FileWriter writer("output.art")); // Makes sense

However, if we instead write to a DynamicMemoryStream, this illustrates the problem the C++ standard was trying to avoid:

artFile.Write(Stream::DynamicMemoryWriter());
// Okay... now what? Where's my written data?

Hence an rvalue overload might make sense when writing, for some streams, but not for all streams.

... maybe that extended explanation should have become a blog post or something.

DanRStevens commented 5 years ago

I've opened Issue #286 to track such changes.