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 custom move operations for ObjectStore::Transaction #7303

Merged
merged 5 commits into from Jan 26, 2016

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Jan 20, 2016

I wrote up a quick unit test to make sure that ObjectStore::Transaction's default move operations were doing what we expected them to, and it turns out that they weren't.

What I learned is that the compiler treats plain-old-data types specially when generating their default move operations. Because the builtin types don't have default initial values, it reverts to a normal copy and makes no effort to restore the source object to default values. See 'Trivial move constructor' at http://en.cppreference.com/w/cpp/language/move_constructor for more information.

I added a custom move constructor and move assignment operator to TransactionData that does restore the default values, and the unit tests pass as expected.

// object to end up zero-initialized, however, so we have to override the
// default implementations to do that
TransactionData(TransactionData&& other) :
ops(other.ops),
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, do we need explicate std::move? otherwise it should be a copy construct?

Copy link
Member

Choose a reason for hiding this comment

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

ignore this. I'm looking in the wrong

@tchaikov
Copy link
Contributor

lgtm

@cbodley
Copy link
Contributor Author

cbodley commented Jan 21, 2016

I think there are some other fields of Transaction that should be reset on move as well, I'll look into that.

@cbodley
Copy link
Contributor Author

cbodley commented Jan 21, 2016

I pushed an updated branch that adds the custom move operations to Transaction as well, to reset its members with builtin types (osr, use_tbl, coll_id, object_id) to their default values.

I considered moving those fields into TransactionData, so we could avoid overriding the move operations in Transaction. But I decided to leave them there, so that we have the move operations in place when people add more fields to Transaction in the future.

Signed-off-by: Casey Bodley <cbodley@redhat.com>
this makes it easier to update constructors when adding/removing fields

Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
liewegas added a commit that referenced this pull request Jan 26, 2016
os/ObjectStore: add custom move operations for ObjectStore::Transaction

Reviewed-by: Kefu Chai <kchai@redhat.com>
@liewegas liewegas merged commit 94ba006 into ceph:master Jan 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants