Skip to content
This repository has been archived by the owner on Sep 22, 2022. It is now read-only.

Add move assignment operator for cursor_managed #270

Closed
wants to merge 1 commit into from

Conversation

canepat
Copy link

@canepat canepat commented Feb 23, 2022

Custom move assignment operator for cursor_managed is required to close the current handle before moving (see also erigontech/silkworm#575).

@erthink
Copy link
Owner

erthink commented Feb 23, 2022

Hmm, I will fix this (by my self) promptly.

@canepat
Copy link
Author

canepat commented Feb 23, 2022

Sorry, I've missed that cursor explicitly manages the handle, so probably the correct solution is a bit more involved.

@erthink
Copy link
Owner

erthink commented Feb 23, 2022

Please check out the d8431a3

@erthink
Copy link
Owner

erthink commented Feb 23, 2022

Minor refine commit' comment by the 3c574fc.

@erthink
Copy link
Owner

erthink commented Feb 23, 2022

@canepat, thank you for reporting.

@canepat
Copy link
Author

canepat commented Feb 23, 2022

These changes raise now compiler errors on code using mdbx in cases like this:

mdbx::txn tx = ...
mdbx::cursor_managed source{tx.open_cursor(...)};
...
source.close();
source = tx.open_cursor(...); // compiler error here

which compiled and worked as expected before (the leak was present just if close is not called). I don't think that your goal is prohibiting the cursor_managed move assignment or am I missing something?

Apart from compiler errors, using the inherited move assignment operator in cursor_managed class seems insufficient to me because the base class operator does not dispose the handle. Isn't the cursor_managed class supposed to manage the handle implicitly in all cases?

The change proposed in this PR doesn't break the case listed above and also closes the handle automatically when moving if close has not been called explicitly.

@canepat
Copy link
Author

canepat commented Feb 23, 2022

I've seen now your rework in 464886a and it looks good to me, it should solve my issues in previous comment.

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

Successfully merging this pull request may close these issues.

2 participants