-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Issue 598: Refactoring List class #702
Conversation
🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats. |
…e/public), adapt docstrings according to changed method signatures
…ontains() a const method
@gavv I would appreciate your feedback to this PR. Thanks! |
Hi, thanks for PR! A few comments:
|
…ages of container_of from ListImpl to List, make container_of function private again
…NodeData, change deconstructor of List, make head private and add getter, nulling list of head in ListImpl, making check_is_member private
Hi @gavv,
I did this at the cost of implementing I would appreciate your review! |
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.
Thanks for update! Some final comments.
} | ||
|
||
head_.list = NULL; | ||
impl_.~ListImpl(); |
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.
This way impl dtor is called twice. There is no need to call it explicitly, you can just remove this line.
if (data) | ||
return static_cast<T*>(data->container_of()); | ||
else | ||
return NULL; | ||
} |
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.
Please always use {}
.
//! Get head of list | ||
ListNode::ListNodeData head() const; |
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.
I guess we should return a reference or a pointer here. It's expected that you can take address of any node in list.
private: | ||
size_t size_; | ||
//! Head of list | ||
ListNode::ListNodeData _head; |
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.
nit: _head
=> head_
//! Check if list node data is registered in this list. | ||
static void check_is_member(const ListNode::ListNodeData* data, const ListImpl* list); |
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.
nit: check_is_member
=> check_is_member_
(because it's private)
impl_.push_back(element.list_node_data()); | ||
|
||
OwnershipPolicy<T>::acquire(element); |
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.
impl_.insert(element.list_node_data(), &impl_.head());
this should work (if you'll return a reference from head()) - and impl.push_back won't be needed.
🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts. |
Ported from roc-streaminggh-702 Co-authored-by: Veronika Rovnik <[email protected]>
@veronikaro Hi, my recent commit (2510edd) introduced lots of conflicts with this patch, so I ported your code to the new version and finished remaining issues: bef7210 Thanks! |
Solves #598.