Skip to content

Conversation

@nielsdos
Copy link
Member

No description provided.

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.

Seems sensible to me

@nielsdos nielsdos requested a review from Girgias March 15, 2025 09:30
@nielsdos
Copy link
Member Author

Ah great, now Windows fails ffs

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.

Still LGTM

ext/xml/xml.c Outdated
Comment on lines 676 to 679
zend_string *stripped_tag = xml_stripped_tag(tag_name, parser->toffset);
xml_add_to_info(parser, stripped_tag);

add_assoc_string(&tag, "tag", skipped_tag_name);
add_assoc_str(&tag, "tag", stripped_tag);
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to add a comment to indicate that you are transferring the lifetime of the str to the tag array?

}
} else {
char *skipped_tag_name = SKIP_TAGSTART(ZSTR_VAL(tag_name));
zend_string *stripped_tag = xml_stripped_tag(tag_name, parser->toffset);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

ext/xml/xml.c Outdated
Comment on lines 887 to 889
zend_string *stripped_tag = xml_stripped_tag(parser->ltags[parser->level - 1], parser->toffset);
xml_add_to_info(parser, stripped_tag);
add_assoc_str(&tag, "tag", stripped_tag);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@nielsdos nielsdos merged commit c5f6a8b into php:master Mar 15, 2025
1 check passed
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.

2 participants