Issue 598: Refactoring List class #702

veronikaro · 2024-02-19T23:17:27Z

Solves #598.

github-actions · 2024-02-19T23:17:41Z

🤖 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

veronikaro · 2024-02-25T23:07:04Z

@gavv I would appreciate your feedback to this PR. Thanks!

gavv · 2024-02-28T08:41:27Z

Hi, thanks for PR!

A few comments:

Please fix CI. There is a crash in tests reported by several jobs. (You can reproduce it locally by using --sanitizers option).
Some ListImpl methods use ListNodeData, and some use ListNode. For consistency, I suggest to use ListNodeData everywhere.
I think we could keep container_of_() in template class, after all ListImpl methods will switch to ListNodeData.
ListImpl::is_empty() can be removed. We can implement List::is_empty() using ListImpl.size().
We can simplify ~List() to something like (see below why):
```
while (!is_empty()) {
    pop_front();
}
```
Also we can move impl_.head.list = NULL; from ~List() to ~ListImpl(). Since ListImpl is responsible to initialize head, it's logical that it's also responsible to deinitialize it.
After these changes we can make check_is_member() a private method of ListImpl.
Since ListImpl is a class with methods, not a C-like struct, for consistency I suggest to make head field private and add a public getter ListNodeData& head().

…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

veronikaro · 2024-04-01T21:07:32Z

Hi @gavv,
sorry for the delay.
I addressed all your comments, thank you very much for the feedback.

8. Since ListImpl is a class with methods, not a C-like struct, for consistency I suggest to make head field private and add a public getter `ListNodeData& head()`.

I did this at the cost of implementing push_back as a function of ListImpl level. The reason for this is that push_back requires a pointer to the head.

I would appreciate your review!

gavv

Thanks for update! Some final comments.

gavv · 2024-04-09T18:58:09Z

src/internal_modules/roc_core/list.h

        }
-
-        head_.list = NULL;
+        impl_.~ListImpl();


This way impl dtor is called twice. There is no need to call it explicitly, you can just remove this line.

gavv · 2024-04-09T18:59:31Z

src/internal_modules/roc_core/list.h

+        if (data)
+            return static_cast<T*>(data->container_of());
+        else
+            return NULL;
    }


Please always use {}.

gavv · 2024-04-09T19:03:07Z

src/internal_modules/roc_core/list_impl.h

+    //! Get head of list
+    ListNode::ListNodeData head() const;


I guess we should return a reference or a pointer here. It's expected that you can take address of any node in list.

gavv · 2024-04-09T19:03:21Z

src/internal_modules/roc_core/list_impl.h

+private:
+    size_t size_;
+    //! Head of list
+    ListNode::ListNodeData _head;


nit: _head => head_

gavv · 2024-04-09T19:03:39Z

src/internal_modules/roc_core/list_impl.h

+    //! Check if list node data is registered in this list.
+    static void check_is_member(const ListNode::ListNodeData* data, const ListImpl* list);


nit: check_is_member => check_is_member_ (because it's private)

gavv · 2024-04-09T19:07:12Z

src/internal_modules/roc_core/list.h

+        impl_.push_back(element.list_node_data());
+
+        OwnershipPolicy<T>::acquire(element);


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.

github-actions · 2024-05-14T10:29:08Z

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

Ported from roc-streaminggh-702 Co-authored-by: Veronika Rovnik <veronikarovnik@gmail.com>

gavv · 2024-05-15T17:17:26Z

@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!

Refactoring List class: draft version

87898c9

github-actions bot added the work in progress Pull request is still in progress and changing label Feb 19, 2024

gavv added the contribution A pull-request by someone else except maintainers label Feb 20, 2024

veronikaro added 6 commits February 22, 2024 16:16

Correct member function name since it is public

b888ac0

Adapt names of class members according to their access levels (privat…

44d1933

…e/public), adapt docstrings according to changed method signatures

Make head of List class private and adapt usages accordingly

9b68ae6

Documentation fix

ff2f0a2

Correct docstring

4343682

Correct check_is_member function

1f882b3

veronikaro changed the title ~~Refactoring List class~~ Issue 598: Refactoring List class Feb 25, 2024

Correct docstrings to methods; adapt check_is_member function; make c…

c89f86b

…ontains() a const method

veronikaro marked this pull request as ready for review February 25, 2024 23:08

github-actions bot added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing labels Feb 25, 2024

gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Feb 28, 2024

veronikaro added 5 commits March 3, 2024 14:50

Improve documentation

6d182f1

Add forgotten registering of list in ListNodeData

5339306

Change signatures of ListImpl methods to return ListNodeData; move us…

70a41dd

…ages of container_of from ListImpl to List, make container_of function private again

Leave is_empty method only in List class

b1500e7

Deinitialize head in ListImpl class

d76a899

gavv added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Mar 4, 2024

veronikaro added 2 commits April 1, 2024 22:59

Addressing review comments: adapting ListImpl methods operate on List…

ee3cd31

…NodeData, change deconstructor of List, make head private and add getter, nulling list of head in ListImpl, making check_is_member private

Merge remote-tracking branch 'upstream/develop' into issue_598

80eb1b5

Fix documentation to let CI pipeline run

52897ec

Fix documentation

bc5cc66

gavv reviewed Apr 9, 2024

View reviewed changes

gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Apr 9, 2024

github-actions bot added the needs rebase Pull request has conflicts and should be rebased label May 14, 2024

gavv added a commit to gavv/roc-toolkit that referenced this pull request May 15, 2024

roc-streaminggh-598 Extract ListImpl class

bef7210

Ported from roc-streaminggh-702 Co-authored-by: Veronika Rovnik <veronikarovnik@gmail.com>

gavv closed this May 15, 2024

gavv added merged manually and removed needs revision Pull request should be revised by its author labels May 15, 2024

github-actions bot removed the needs rebase Pull request has conflicts and should be rebased label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 598: Refactoring List class #702

Issue 598: Refactoring List class #702

veronikaro commented Feb 19, 2024 •

edited

github-actions bot commented Feb 19, 2024

veronikaro commented Feb 25, 2024

gavv commented Feb 28, 2024 •

edited

veronikaro commented Apr 1, 2024

gavv left a comment

gavv Apr 9, 2024

gavv Apr 9, 2024

gavv Apr 9, 2024

gavv Apr 9, 2024

gavv Apr 9, 2024

gavv Apr 9, 2024

github-actions bot commented May 14, 2024

gavv commented May 15, 2024 •

edited

		//! Check if list node data is registered in this list.
		static void check_is_member(const ListNode::ListNodeData* data, const ListImpl* list);

		impl_.push_back(element.list_node_data());

		OwnershipPolicy<T>::acquire(element);

Issue 598: Refactoring List class #702

Issue 598: Refactoring List class #702

Conversation

veronikaro commented Feb 19, 2024 • edited

github-actions bot commented Feb 19, 2024

veronikaro commented Feb 25, 2024

gavv commented Feb 28, 2024 • edited

veronikaro commented Apr 1, 2024

gavv left a comment

Choose a reason for hiding this comment

gavv Apr 9, 2024

Choose a reason for hiding this comment

gavv Apr 9, 2024

Choose a reason for hiding this comment

gavv Apr 9, 2024

Choose a reason for hiding this comment

gavv Apr 9, 2024

Choose a reason for hiding this comment

gavv Apr 9, 2024

Choose a reason for hiding this comment

gavv Apr 9, 2024

Choose a reason for hiding this comment

github-actions bot commented May 14, 2024

gavv commented May 15, 2024 • edited

veronikaro commented Feb 19, 2024 •

edited

gavv commented Feb 28, 2024 •

edited

gavv commented May 15, 2024 •

edited