Add string view support (only impl, no tests)

[brandl-muc]

Productive implementation should be complete (restricted to the functions you mentioned for a first PR) but there are no tests yet. So this is intentionally only a draft to get quick review on the implementation. Despite the limited scope there are still many choices and you might want those differently. Examples are:

• Issue a warning if string_view is requested or not?
• Delegate to existing overload or copy the implementation?
• Define string_view_t via a type alias since we already know this is supported or use typedef to stay consistent with char_t etc.?

I will add tests soon and then publish the PR.

[zeux]

This is a good isolated starting point (modulo tests and adding the define to GitHub Actions / AppVeyor config). I think it's fine to delegate to existing overloads when they are available; these details could be changed later if need be (perhaps for debug performance, although using string_view necessitates function calls in debug builds anyhow).

[brandl-muc]

Regarding "adding the define to GitHub Actions / AppVeyor config", this seems to be the next step, before adding unit tests. A gave it a shot but you will probably not like it, so please help with that.

For gcc/clang my approach seems to work, for Windows I'm not so sure: (and I can't test locally)

CMake Warning:
  Manually-specified variables were not used by the project:

    PUGIXML_STRING_VIEW
    standard

The warning is not new, but now it also includes PUGIXML_STRING_VIEW.

[zeux]

For gcc/clang my approach seems to work, for Windows I'm not so sure: (and I can't test locally)

For all of them, this should be an expansion of the existing matrix (just need to add this to defines). For Makefile builds, you should not need to change the Makefile itself. defines is already providing you what you need. You do indeed need to add a make invocation with c++17 standard, probably debug config is better because that will maybe enable more assertions and be faster to build. For CMake builds, you need to add handling of this define to CMakeLists.txt; unsure if you need to override the standard version for MSVC to work, but that can be done via CMAKE_CXX_STANDARD externally if necessary I guess. Or the CMakeLists can be changed to set the standard to 17 if PUGIXML_STRING_VIEW was requested. You should be able to test all this locally via CMake builds, even if you don't have Windows.

[brandl-muc]

But wouldn't adding PUGIXML_STRING_VIEW to the matrix never build it in combination with say PUGIXML_WCHAR_MODE?

[zeux]

If you want that combination I think it can be added by adding "PUGIXML_WCHAR_MODE,PUGIXML_STRING_VIEW". In general pugixml doesn't test all permutations on GHA as there are too many; I could see an argument for this specific combination being more valuable than most I guess, although I would also assume that we don't have to special case this in the code necessarily.

[brandl-muc]

You're probably right, WCHAR_MODE is not really that different when it comes to the string_view changes. I'll try to rework the integration soon.

[dantargz]

Hi, it looks like this PR has gotten stuck. I took a stab at adapting all of the tests that were added for the char*+size versions of the functions to also test string_view, updating the cmake files, and updating the workflow files to add coverage.

I am new to making open source contributions, I didn't realize that making a draft pr in my fork would add a message to this thread, sorry for the noise. Let me know if I can help in some way, like by merging any testing / cmake / build.yml changes into brandl's fork for inclusion in this PR.

[brandl-muc]

Hi @dantargz, you mentioned this PR in the description of your PR, that's what caused "the noise". I hadn't forgotten this PR but apple harvest (I live on a small former farm) required my spare time. But help of course is appreciated, I would drop mine when your PR gets merged.

[dantargz]

thanks, I opened a PR against the main repo here: https://github.com/zeux/pugixml/pull/633

[zeux]

Merged as #633, thanks!