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

Add setAutoCommit method to the Transaction class #1459

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

an-tao
Copy link
Member

@an-tao an-tao commented Dec 20, 2022

@an-tao an-tao requested a review from marty1885 April 1, 2023 15:23
thisPtr->commitCallback_(true);
}
},
[thisPtr](const std::exception_ptr &) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the last place we used thisPtr. Can move move it to avid a copy?

Comment on lines +44 to +45
commitCb = std::move(commitCallback_),
autoCommit = autoCommit_]() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UB if the user manually commits then auto commit happens. Could the following happen?

auto t = db->newTransactionCoro();
...
t->commit();
// now `t` destructs. Calls auto commit. We try to move an already moved variable.

{
commitCb(true);
}
},
[commitCb](const std::exception_ptr &ePtr) {
[commitCb, autoCommit](const std::exception_ptr &ePtr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the last time we use commitCb. We can move it instead of making a copy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the arguments evaluated in left-to-right order? If not, we can not use move.

@an-tao an-tao requested a review from hwc0919 April 15, 2023 01:21
@an-tao an-tao marked this pull request as draft April 8, 2024 08:27
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 this pull request may close these issues.

None yet

3 participants