Skip to content

Add tidyNode::getNextSibling() and tidyNode::getPreviousSibling() #15047

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

First commit refactors the code a bit to make implementation easier.

Second commit actually implements it:
These get the next and previous sibling nodes, respectively.
We can already kind of do this by using the $child array, but that's
inconvenient when actually walking the tree by only using node
instances. Since the class is final, there is no BC break here.

newobj->ptdoc->ref_count++;
tidy_add_node_default_properties(newobj);
} else {
ZVAL_NULL(return_value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just lost here ... not necessary anymore right ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You never need to set the return value to IS_NULL because the VM & JIT do it prior to the call.
I removed it to reduce the number of lines that are very very similar.

@nielsdos
Copy link
Member Author

FWIW I sent an email to internals to ask if anyone objects: https://externals.io/message/124530

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge the first commit.

Don't forget to add an UPGRADING note, but I think this is a good addition will also reply on the ML.

These get the next and previous sibling nodes, respectively.
We can already kind of do this by using the $child array, but that's
inconvenient when actually walking the tree by only using node
instances. Since the class is final, there is no BC break here.
@coogle
Copy link

coogle commented Jul 22, 2024

This looks like a useful addition to the extension to me.

@coogle coogle self-requested a review July 22, 2024 17:59
Copy link

@coogle coogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nielsdos nielsdos closed this in ad45208 Jul 25, 2024
@nielsdos
Copy link
Member Author

I merged this as no one complained and reaction was positive. Thanks for checking!

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

Successfully merging this pull request may close these issues.

4 participants