Skip to content
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

intrusive_forward_list: lifetime of forward_links are attached to that of intrusive_forward_list ? #977

Open
s-t-a-n opened this issue Nov 16, 2024 · 0 comments

Comments

@s-t-a-n
Copy link

s-t-a-n commented Nov 16, 2024

Hi!

I might be missing something altogether, but I can't wrap my head around certain design decisions of the intrusive_forward_list:

The lifetime of the nodes is external to intrusive_forward_list, as indeed it shouldn't be. Yet the only way to use the forward_links is in a intrusive_forward_list (and variants?), as all construction logistics are handled by this class.

As usual, we want to keep the memory overhead of a list to a bare minimum, so I would like to construct the list using the intrusive_forward_list and then pass the root forward_link onwards to a consumer. If need be, it can locally add the node into back into a list there for iteration/manipulation. If I'd pass around the list itself, I would add some 12 bytes of unnecessary baggage and besides, the copy/move constructors are made private (which makes sense if I could take out the constructed links).

Two problems:

  1. Upon destruction of the intrusive_forward_list, all links between nodes are cleared.
  2. As a terminating leaf, the class uses a static node terminator instead of nullptr

This effectively does tie the lifetime of the linkage of nodes to the forward_list.

I don't understand why? Wouldnt it make much more sense to use the intrusive_forward_list for it's list properties and leave the links as be if the list destructs? The actual list is maintained by the references contained in the forward_links, as one expects from a node in a graph. Shouldn't the list provide purely access such as push/pop/erase/iteration/etc?

Why the terminator? Doesn't that only make sense in a bidirectional list? I see it only being used to compare against, and it seems that nullptr would suffice.

One rather ugly fix would be to add a detach or extract function such that one can construct the links through the intrusive_forward_list yet extract the root node out to pass around to an upstream. Yet this would mean that inevitably one would need to traverse the tree and replace the terminator node with a nullptr.

//*************************************************************************
/// Extracts the root node from the list, effectively clearing the list without altering links.
/// Returns a pointer to the extracted root node.
//*************************************************************************
pointer detach()
{
  auto head = this->get_head();
  while (head->etl_next != &this->terminator)
    head = head->etl_next;
  head->etl_next = nullptr;

  pointer root = static_cast<pointer>(this->get_head());
  this->start.etl_next = &this->terminator;
  this->current_size = 0;

  return root;
}

So two design questions:

  1. Why does the intrusive_forward_list remove the links upon destruction?
  2. Why the terminator leaf node on a single directional list?

If this is by design, this might be good to document as the documentation explicitly states lifetime of nodes is not affected by a call to clear(). If this is a shortcut left over from the bidirectional_link design, it might be good to change this and it shouldn't break the API. I'll gladly contribute to a fix. What do you guys think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant