-
Notifications
You must be signed in to change notification settings - Fork 721
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
std::string and std::string_view support in the same way for setters and constructors as for const char * #73
Comments
This might become much more interesting when the standard library will have |
Here's some previous discussion on the topic: https://code.google.com/p/pugixml/issues/detail?id=49 That thread also has different implementations of a possible solution. |
I experimented a bit with string_arg approach (the link above appears to be dead, but basically one way to implement this is to replace all existing The biggest issue apart from breaking binary compatibility (which would mean I'd like to defer this until pugixml 2.0 to avoid potential issues with existing applications that link to pugixml dynamically) is the overload issues with Overall this seems like a good solution. There's obviously extra overhead in debug builds but there is no cost if compiler inlines the helper (which it should when optimizations are enabled). |
It would be better to support std::string_view, it will cover any strings and will solve problem with using unterminated strings. Btw - great library i love projects which have stl compatibile interface |
A smaller but still useful change would be to have a |
I think std::(w)string_view is much more desirable as std::(w)string. And constructing a string_view from a std::string is very cheap, however constructing a string from a string_view is very expensive, so I would skip the std::string implementation and favor the std::string_view implementation. |
@air2 The question you would need to ask yourself is if you need a null-terminated string or not. Because if that is the case then The other issue would require those who use this library to require a compliant C++17 compiler. With |
It is not required to use C++17. Libraries such as fmt and spdlog (which uses fmt) use a custom tailored |
@matt77hias But that would require the dependencies of an external library which I feel defeats the purpose of this library having to work on it's own without any dependencies except for the C headers that it uses. |
I did not mean to add a dependency. A string_view is just a data pointer
and data count providing some implicit/explicit constructors and cast
operators. It is trivial and lightweight to implement yourself (one does
not need all the other functionality) and one can replace it with
std::string_view once C++17 is supported.
…On Fri, Feb 14, 2020, 00:42 Raul Ramirez ***@***.***> wrote:
@matt77hias <https://github.com/matt77hias> But that would require the
dependencies of an external library which I feel defeats the purpose of
this library having to work on it's own without any dependencies except for
the C headers that it uses.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#73?email_source=notifications&email_token=AASZSE653KI7UDH5XHRN7DLRCXLHJA5CNFSM4BW5HIAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELXA5EY#issuecomment-586026643>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AASZSEZURN4YTEDVGRGNJNLRCXLHJANCNFSM4BW5HIAA>
.
|
Is there any progress on this? As mentioned in #73 (comment) there is no need to depend on any new functionality, just provide an overload which accepts size parameter (which already is done internally). It is not always input value has terminating null and adding one means creating an unnecessary copy on heap. |
That's why you need |
This one halx99#2 done setter/getter with string_view and other improvements, but the |
7 years passed :) Is it feasible to propose a pull request with the necessary changes? Will it be accepted? pugixml performance will be better with string and string_view, why it wasn't considered yet? |
@zeux maybe there should be separate branch for possibly breaking string_view-friendly API for people who need it? I am geniunely consider dropping or forking pugixml due to being unable to use |
A C++ library should have a modern and fast C++ interface... |
Please keep the comments on topic and substantive. In a recent change, several methods (like set_value and xml_text::set) gained support for size_t argument. To what extent adding string_view support just for these, and keeping functions that deal with attribute/element names as they are, would satisfy the need here? It's of course possible to add string_view overloads for every method. That doubles the interface size for the library, and requires alternate implementations in certain cases where currently the internals assume null terminated inputs - this is obviously possible, but also not ideal. The original idea for this issue has been to replace all methods with something that uses a wrapper, but given the binary compatibility concerns and uncertain timeline for 2.0 it's not clear that this is actually optimal. The reason why names might be special is that code is generally more likely to use string literals than dynamic strings for them. |
@zeux For me personally, I don't really care about supporting Perfect solution for me would be to have optional |
As mentioned above, certain functions like |
I have My issue is that I am effectively forbidden from using |
For the wrapper library use case it feels fairly easy to implement GetChild(name) as something like
(I do think that for direct use for people who are used to string_view it would be more ergonomic to support it directly as right now you need to use .data()/.size() and call the overload that supports size which isn’t ideal) edit: the above doesn’t cover set_name, so that probably does need an extra overload (now part of master). |
Hi and thanks for amazing library!
Do you plan to add std::string and std::string_view overloads for most setters/constructors? It makes code usage much more convenient.
The text was updated successfully, but these errors were encountered: