arximboldi / lager

C++ library for value-oriented design using the unidirectional data-flow architecture — Redux for C++
https://sinusoid.es/lager/
MIT License
704 stars 66 forks source link

Allow forward declaration of type T in reader<T> class member #185

Closed TheCoconutChef closed 1 year ago

TheCoconutChef commented 1 year ago

Consider:

#include <lager/reader.hpp>

struct Foo;

struct Bar {
    lager::reader<Foo> foo_reader;
};

struct Foo {};

as of right now, this will not compile due to the use of value_type current_/last_ in reader_node<T>.

However, if we swap value_type for, say, std::unique_ptr<value_type> with appropriate modification to the ctor, the last() method, the current() method, and the various push/send method, it seemingly "just work".

I understand that changing substituting std::unique_ptr<value_type> for value_type would require extra allocation at reader_node construction, which is a downside. However, the ability to forward declare T of reader<T> might be worth it? Was the inability to do so ever a problem, or are there other reasons that would recommend against such a change?

tusooa commented 1 year ago

I wonder whether that's really possible as reader.get() has to return a value type instead of a reference?

TheCoconutChef commented 1 year ago

@tusooa My understanding is that reader.get() returns a const T&, as I believe we can see from https://godbolt.org/z/WaaMYxKza. The reader_mixin on which the function is defined does indeed seem to return a call to the last() method which does return a const value_type&.

arximboldi commented 1 year ago

This could be indeed a very nice feature...

TheCoconutChef commented 1 year ago

After benchmarking the changes I mentioned above, I measured a ~40% performance decrease in the creation of a random DAG made up of readers. Considering that changing the type of current_ and last_ for a unique_ptr<value_type> would not be opt-in, and considering that what I'm looking for can basically be achieved in an opt-in manner through the use of a pimpl in the client class, I think I'm gonna close the issue.

For reference, here are the changes that allowed forward declaration of T in the context of a class member lager::reader<T>: https://github.com/TheCoconutChef/lager/commit/4a088ab63ca92e6ee4f00bd120e552ad842f5a14

arximboldi commented 1 year ago

Thank you so much for the investigation @TheCoconutChef !