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

Add remaining std::string_view overloads #636

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

dantargz
Copy link
Contributor

@dantargz dantargz commented Oct 23, 2024

Adding overloads for string_view to core public APIs as described in #626

The most material change is adding a helper function for checking equality between string_view and null terminated c-strings impl::stringview_equal. Using strncmp/wsncmp directly would not work, because that stops comparison at the first null, and the string_view might have a null in the middle -- we do not want a string_view of "n\0pe" to be considered equal to a node named "n" for lookups. string_view has a compare function that has an overload for const char* (null terminated c-string) that would give the comparison we want. However, that helper works by constructing a string_view from the const char* and then comparing to that new string_view, which would trigger an extra iteration over the const char*. The existing helper strequalrange also cannot be used: if the string_view is "n\0pe" and the test name is "n", the n and null characters will match, and then the function will attempt to read past the end of the allocation of the test name.

These overloads are created by copying and pasting the original and substituting the new equality function:

xml_node xml_node::child(string_view_t name) const;
xml_attribute xml_node::attribute(string_view_t name) const;
xml_node xml_node::next_sibling(string_view_t name) const;
xml_node xml_node::previous_sibling(string_view_t name) const;
xml_attribute xml_node::attribute(string_view_t name, xml_attribute& hint) const;

Added some unit tests for child(string_view_t) and attribute(string_view_t). Also added unit tests documenting the behavior of child and attribute lookup when the node/attribute has null name -- searching does not find these nodes, even if an empty argument name is provided.

These other functions also gained overloads, but they just pass the string_view_t to another overloaded function. No new unit tests were added for these functions.

xml_attribute& xml_attribute::operator=(string_view_t rhs);

xml_attribute xml_node::append_attribute(string_view_t name);
xml_attribute xml_node::prepend_attribute(string_view_t name);
xml_attribute xml_node::insert_attribute_after(string_view_t name, const xml_attribute& attr);
xml_attribute xml_node::insert_attribute_before(string_view_t name, const xml_attribute& attr);
xml_node xml_node::append_child(string_view_t name);
xml_node xml_node::prepend_child(string_view_t name);
xml_node xml_node::insert_child_after(string_view_t name, const xml_node& node);
xml_node xml_node::insert_child_before(string_view_t name, const xml_node& node);
bool xml_node::remove_attribute(string_view_t name);
bool xml_node::remove_child(string_view_t name);

xml_text& xml_text::operator=(string_view_t rhs);

Documentation will be updated to cover the new overloads after the opt-in define PUGIXML_STRING_VIEW is removed

@dantargz dantargz marked this pull request as ready for review October 24, 2024 00:55
@zeux
Copy link
Owner

zeux commented Oct 24, 2024

Thanks! Would you mind also replicating dom_node_attribute_hinted test for string_view, as this is the only function with non-trivial logic that had to be copied? The code changes look good to me. There might be some missing coverage to sort out in the future, but the other functions are trivial so they should be correct.

@dantargz
Copy link
Contributor Author

I will add the clone test for dom_node_attribute_hinted. I am also going to add simple unit tests for the rest of the functions so that there is basic coverage for everything, let me know if you would prefer that the tests be added in a separate PR to keep the size of this PR low / reduce churn.

@zeux
Copy link
Owner

zeux commented Oct 24, 2024

As long as the changes are test-only it's fine to add in this PR.

@zeux zeux merged commit 13beda2 into zeux:master Oct 24, 2024
27 checks passed
@zeux
Copy link
Owner

zeux commented Oct 24, 2024

Merged, thanks a lot!

@dantargz dantargz deleted the issue-626-add-stringview-remainder branch October 25, 2024 17:18
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

Successfully merging this pull request may close these issues.

2 participants