implement exec.split #81
Conversation
maikel
force-pushed
the
exec.split
branch
2 times, most recently
from
a14ccc2
to
b16afd5
Compare
| //! | ||
| //! @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
| local_state_base(const local_state_base&) = delete; | ||
| local_state_base(local_state_base&&) = delete; | ||
| auto operator=(const local_state_base&) -> local_state_base& = delete; | ||
| auto operator=(local_state_base&&) -> local_state_base& = delete; |
There was a problem hiding this comment.
I have recently introduced a base class ::beman::execution26::detail::virtual_immovable which provides this boiler plate: some of the clang-tidy checks insisted in that stuff being written which is quite annoying. The class is currently possibly in the wrong header (immovable.hpp).
There was a problem hiding this comment.
Note that I think it is more "efficient" to define a protected destructor and make the derived class final. But I can just switch to virtual immovable.
NB. I run clang-tidy and cppcheck locally and it still complains on classes using virtual_immovable. Although I have to check which one complains. Would be good to use the same tooling consistently in CI and local dev.
|
Quick feedback is great. I think i should expand the tests anyway to cover more properties and usecases. |
This PR tracks my progress in implementing exec.split.