-
Notifications
You must be signed in to change notification settings - Fork 93
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 copy constructor to ColumnFamilyHandleImpl #353
Conversation
Signed-off-by: Yang Zhang <[email protected]>
Signed-off-by: Yang Zhang <[email protected]>
@@ -50,6 +50,14 @@ ColumnFamilyHandleImpl::ColumnFamilyHandleImpl( | |||
} | |||
} | |||
|
|||
ColumnFamilyHandleImpl::ColumnFamilyHandleImpl( | |||
const ColumnFamilyHandleImpl& other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just move the other?
Signed-off-by: Yang Zhang <[email protected]>
Signed-off-by: Yang Zhang <[email protected]>
db/column_family.cc
Outdated
@@ -50,6 +50,15 @@ ColumnFamilyHandleImpl::ColumnFamilyHandleImpl( | |||
} | |||
} | |||
|
|||
ColumnFamilyHandleImpl::ColumnFamilyHandleImpl(ColumnFamilyHandleImpl&& other) { | |||
other.cfd_ = cfd_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so that means the new ColumnFamilyHandleImpl instance will not have the valid information, but the other
will be overwritten by the default values of ColumnFamilyHandleImpl?
I would imagine other
will be empty and it's moved to this
instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I didn't pay attention to copilot generated code.
Signed-off-by: Yang Zhang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Yang Zhang <[email protected]>
Signed-off-by: Yang Zhang <[email protected]>
Signed-off-by: Yang Zhang <[email protected]>
Ref tikv/titan#294
This is needed by tikv/titan#298.
In that PR, we need to create Titan's own
ColumnFamiliyHandle
instances. To avoid memory leak, we need to deleteColumnFamilyHandleImpl
instances created by RocksDB itself.