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

deprecation of the stream_to constructor #706

Closed
belst opened this issue Jun 13, 2023 · 7 comments · Fixed by #707
Closed

deprecation of the stream_to constructor #706

belst opened this issue Jun 13, 2023 · 7 comments · Fixed by #707

Comments

@belst
Copy link

belst commented Jun 13, 2023

Hi,

I wanted to update the library to the latest version, but now I get deprecation warnings when using the stream_to constructor.

The static factory methods do not really work for me, since stream_to is not movable, so I cannot store it in an optional.

I guess I could write a wrapper like this:

struct stream_to_wrapper {
    stream_to_wrapper(
        transaction_base& tx,
        table_path path,
        std::initializer_list<std::string_view> columns = {}
    ) : to(stream_to::table(tx, path, columns)) {}

    stream_to to;
};

// Use like:
std::optional<stream_to_wrapper> s;
s.emplace(tx, table, columns);

(EDIT: this wrapper unfortunately does not work)

I have a database class which can only ever have 1 writer active, so I store it in an optional and keep writing incoming data until I manually commit it.

Is there a way to store the writer in an optional without using a wrapper struct?

@jtv
Copy link
Owner

jtv commented Jun 14, 2023

ISTM we could make stream_to movable.

@jtv
Copy link
Owner

jtv commented Jun 19, 2023

It's not as easy as it seems. We need a pointer to its transaction_focus sub-object, and that pointer will break if we move the stream.

But perhaps with some trickery in the move constructor...

jtv added a commit that referenced this issue Jun 19, 2023
Should help with #706, though we may need move assignment as well.

Finally biting the bullet and adding move construction to
`transaction_focus`, which requires dealing with that backpointer from
the transaction back to the focus object.

I'll try move assignment next.  The obvious complication there is that
`m_trans` will have to become a pointer.
@jtv
Copy link
Owner

jtv commented Jun 19, 2023

I think I've cracked it. @belst could you try it with the branch in #707?

@belst
Copy link
Author

belst commented Jun 20, 2023

Just replacing my current pointer based setup with optional::emplace seems to work. Thanks. Haven't done many more tests yet tho.

@jtv
Copy link
Owner

jtv commented Jun 20, 2023

Please throw any testing at it that you can. I'm still a bit nervous about the way I had to std::move() an object just to get the base-class object, and then std::move() some of its members as well. A fun little puzzle.

@jtv
Copy link
Owner

jtv commented Jun 21, 2023

A next step could be to support moving of more transaction_focus-derived classes.

In my test I used std::in_place for constructing the std::optional. I'll keep this project open for a while, so I can incorporate any further notes you may have @belst.

jtv added a commit that referenced this issue Jun 22, 2023
Should help with #706, though we may need move assignment as well.

Finally biting the bullet and adding move construction to
`transaction_focus`, which requires dealing with that backpointer from
the transaction back to the focus object.

I'll try move assignment next.  The obvious complication there is that
`m_trans` will have to become a pointer.
@jtv
Copy link
Owner

jtv commented Jul 6, 2023

OK, it looks pretty solid to me, it's got a test, and I've now expressly blocked any move constructors in the hierarchy that I didn't want (previously blocked by transaction_focus base class not having one). I'll merge the PR.

@jtv jtv closed this as completed in #707 Jul 6, 2023
jtv added a commit that referenced this issue Jul 6, 2023
Fixes #706.

Finally biting the bullet and supporting moves of transaction_focus, which requires dealing with that backpointer from the transaction back to the focus object.
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

Successfully merging a pull request may close this issue.

2 participants