implement exec.split #81
Conversation
maikel
force-pushed
the
exec.split
branch
3 times, most recently
from
80f8df1 to
613298c
Compare
dietmarkuehl
left a comment
dietmarkuehl
left a comment
There was a problem hiding this comment.
Last week I didn't get around doing any review (the C++ committee meeting was busy till late on all the days) and I haven't reviewed all the code, yet. However, I have a few comments.
| //! | ||
| //! @tparam Item The type of the item in the stack. | ||
| //! @tparam Next The pointer to the next item in the stack. | ||
| template <class Item, Item* Item::*Next> |
There was a problem hiding this comment.
In the past compilers were pretty bad optimising anything around pointer to members. They tend to be way better at optimising around inline functions and the parameter could be something like
decltype([](auto* node){ return node->next; })
This lambda could also be a default argument if the objective is avoiding the need to specify it. Admittedly, using this approach is more involved when actually using the data structure. This comment is primarily for consideration and doesn't necessarily need to be addressed.
There was a problem hiding this comment.
I will investigate code gen. Now I am curious
| } else { | ||
| assert(!head_ && !last_); | ||
| head_ = item; | ||
| last_ = item; |
There was a problem hiding this comment.
It seems this assignment needs to be outside of the else-branch: if last_ wasn't nullptr before the operation, item also becomes the new last_ element.
There was a problem hiding this comment.
I guess this class implementation is not sound and wrong for the general case. In split I just call pop and call it a day but it can't stay like this. I might make this a class containing single pointer only that you can iterate through.
Good catch.
There was a problem hiding this comment.
I think split just needs a list of items and the order doesn't matter. Moving it to an intrusive_stack rather than an intrusive_queue would be fine. I guess the use in split works with the current implementation but other uses, if they energe, might not.
| ~intrusive_queue() { assert(!head_); } | ||
|
|
||
| //! @brief Pushes an item to the queue. | ||
| auto push(Item* item) noexcept -> void { |
There was a problem hiding this comment.
It seems there should either be a precondition assert(not item->next) (if the operation pushes just one element) or before updating last the list of items hanging off item needs to be walked.
There was a problem hiding this comment.
Yes I was lazy and introduced bugs. Whatever I pop in atomic_intrusive_queue is a singly linked list that I want to traverse. I will revise this
|
Quick feedback is great. I think i should expand the tests anyway to cover more properties and usecases. |
| // include/beman/execution26/detail/intrusive_queue.hpp -*-C++-*- | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| #ifndef INCLUDED_BEMAN_EXECUTION26_DETAIL_INTRUSIVE_QUEUE | ||
| #define INCLUDED_BEMAN_EXECUTION26_DETAIL_INTRUSIVE_QUEUE |
There was a problem hiding this comment.
These still use the "queue" vocabulary.
3 participants
This PR tracks my progress in implementing exec.split.