Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
TheCoconutChef opened this issue Jul 6, 2023 · 5 comments
Closed

Comments

@TheCoconutChef
Copy link
Contributor

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
Copy link
Contributor

tusooa commented Jul 6, 2023

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

@TheCoconutChef
Copy link
Contributor Author

@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
Copy link
Owner

This could be indeed a very nice feature...

@TheCoconutChef
Copy link
Contributor Author

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>: TheCoconutChef@4a088ab

@TheCoconutChef TheCoconutChef closed this as not planned Won't fix, can't repro, duplicate, stale Aug 19, 2023
@arximboldi
Copy link
Owner

Thank you so much for the investigation @TheCoconutChef !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants