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 some support for std::string_view #588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ottigeda
Copy link

From previous discussions I read here, I assume you will not accept that, since it requires a newer c++ standard.
Anyway I would like to share this with you and get your opinion on it.

@JWCS
Copy link

JWCS commented Sep 5, 2024

I just discovered that std::string wasn't taken by default for many functions, and this seems to be the only instigation of adding this. For the issue of std::string_view, instead of forcefully setting a higher standard, the string_view lines can be wrapped in # if 's checking if the C++ version >= C++ 17. This would support higher versions, without removing support for lower versions. Is there any reason else why this feature hasn't moved forward?

@zeux
Copy link
Owner

zeux commented Sep 5, 2024

I'm not sure if you're asking about this PR specifically or the larger feature. If you're asking about this PR, it increases the standard version requirements, has conflicts and build issues. If you're asking about std::string support, I don't think that would make sense either, as all existing methods can be called if you use .c_str(), and this doesn't provide benefits outside of minor ergonomics.

However, if you're asking about string_view, I used to have a mostly negative perspective on this but I think it's now shifted; I will create a new issue outlining my current perspective and what PRs I would be happy to see in that world (I don't have time to do any string_view related work myself).

@JWCS
Copy link

JWCS commented Sep 5, 2024

Thanks, I was asking for the larger feature as a whole. I feel like it's worth adding the few extra wrappers/types for string + string_view, in that it reduces friction when using pugixml. As for me, I was sorely confused when I started seeing pages of compile errors saying "no matching function is defined", when I tried to pass "a string" (string or string_view), and I found that this "modern c++" library only accepted c-strings... 😅

If it's as simple as adding more boilerplate functions that end up calling the c-string versions, for every function that has overloads for all the data types, it shouldn't be too difficult? Or do you forsee another architecture issue?

@zeux
Copy link
Owner

zeux commented Sep 5, 2024

I don't think pugixml is ever advertised as "modern C++" library. I will summarize the rest elsewhere and link this PR.

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.

3 participants