Add string view support (only impl, no tests) #627

brandl-muc · 2024-09-10T20:58:50Z

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.

Define PUGI_ENABLE_STRING_VIEW to add string_view overloads when supported by the C++ standard

zeux · 2024-09-11T03:34:22Z

src/pugixml.cpp

@@ -5441,6 +5441,13 @@ namespace pugi
 		return impl::strcpy_insitu(_attr->name, _attr->header, impl::xml_memory_page_name_allocated_mask, rhs, size);
 	}

+#ifdef PUGI_HAS_STRING_VIEW
+	PUGI_IMPL_FN bool xml_attribute::set_name(const string_view_t rhs)


Here and elsewhere const should not be used for arguments as they are passed by value

This is my usual style (as much const as possible), although I would use east const.
I don't have any problems with changing this, but out of curiosity, could you elaborate on your rationale?
The fact that they are passed by value is orthogonal to the fact that the value is not changed.
And because it is not changed it can be made const and IMHO consequently should be made const.

I personally do not subscribe to the "what could be made const should be" when that doesn't affect the interface; in this case, the caller behavior does not change based on whether the argument is const or not. I am aware of the argument that it can help prevent mistakes but I have not seen these happen much in practice so I view this as syntactic noise. You're welcome to disagree of course, but for consistency this const should be removed: pugixml doesn't have a single function with a const parameter that is passed by value, and the only uses of const locals are that where the value is a compile-time const (when this code was written this seemed to enhance readability, whether this is true or not is subjective).

src/pugixml.hpp

zeux · 2024-09-11T03:39:00Z

src/pugixml.hpp

@@ -122,6 +122,26 @@
 #	endif
 #endif

+// Detect if C++ standard supports 'string_view' (2017 or higher)


I would propose simplifying this to enabling internal PUGI_HAS_STRING_VIEW define if PUGIXML_STRING_VIEW define (note the PUGIXML prefix; PUGI_ prefix is for internal macros, PUGIXML_ prefix is for user facing configuration) is enabled, and dropping the _SUPPORTED macro as it does not seem to be useful.

Separately, PUGI_HAS_STRING_VIEW should be undef'd at the end of the header.

PUGIXML_STRING_VIEW should be added, commented out, in pugiconfig.hpp. We can note there that the macro will become unnecessary with future versions.

A question regarding consistency: PUGI_HAS_STRING_VIEW seems to be the only macro using PUGI prefix in the header. All others use PUGIXML, e.g., PUGIXML_HAS_MOVE or PUGIXML_HAS_LONG_LONG which are auto-detected as well. PUGI-macros can only be found in the cpp.
So shouldn't it be PUGIXML_HAS_STRING_VIEW then?

Probably for the same reason it would be the only macro that is actually undefined in the header. (there is no #undef to be found) So I cannot orient on anything. Should I always #undef it even if I didn't defined it? (not that there is a #ifndef PUGI_HAS_STRING_VIEW)

Apologies, I've misled you about the prefix; I forgot the conventions :) Indeed, it should be prefixed with PUGIXML_ and no undef is necessary. I mixed this up with the .cpp file where the prefix is PUGI_IMPL_ and the macros are undef'd to not pollute the namespace in header-only mode.

src/pugixml.hpp

zeux · 2024-09-11T03:41:51Z

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).

This allows to conveniently opt-in to support for std::string_view

No longer define PUGI_STRING_VIEW_SUPPORTED and replace PUGI_ENABLE_STRING_VIEW with PUGIXML_STRING_VIEW

It is C++20 and likely needs <version> to be conditionally included to be defined

brandl-muc · 2024-09-13T20:43:22Z

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 · 2024-09-13T20:51:54Z

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 · 2024-09-13T21:03:54Z

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

zeux · 2024-09-16T23:14:49Z

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 · 2024-09-17T10:42:43Z

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 · 2024-10-22T01:17:49Z

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 · 2024-10-22T06:24:55Z

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 · 2024-10-22T18:28:54Z

thanks, I opened a PR against the main repo here: #633

zeux · 2024-10-22T20:32:17Z

Merged as #633, thanks!

brandl-muc added 3 commits September 9, 2024 22:31

Add macros for string_view support

49383d1

Define PUGI_ENABLE_STRING_VIEW to add string_view overloads when supported by the C++ standard

Define string_view alias for the selected character type

ee28175

Add string_view_t overloads for set_name(), set_value() and set()

f428647

brandl-muc mentioned this pull request Sep 10, 2024

Perspective on std::string_view support #626

Closed

zeux reviewed Sep 11, 2024

View reviewed changes

src/pugixml.hpp Outdated Show resolved Hide resolved

zeux reviewed Sep 11, 2024

View reviewed changes

src/pugixml.hpp Outdated Show resolved Hide resolved

zeux reviewed Sep 11, 2024

View reviewed changes

src/pugixml.hpp Outdated Show resolved Hide resolved

brandl-muc added 8 commits September 13, 2024 20:56

Do not warn if string_view support is requested but not supported

bd0b8e6

Replace type alias with typedef for consistency

2f1be3b

Add commented out define of PUGIXML_STRING_VIEW to pugiconfig.hpp

3e7ecd2

This allows to conveniently opt-in to support for std::string_view

Clean up detection for string_view support

71ad493

No longer define PUGI_STRING_VIEW_SUPPORTED and replace PUGI_ENABLE_STRING_VIEW with PUGIXML_STRING_VIEW

Do not use __cpp_lib_string_view for detection of string_view support

db96936

It is C++20 and likely needs <version> to be conditionally included to be defined

Do not const qualify by-value arguments

16eb5da

Add possibility to pass additional CXXFLAGS to makefile as pugiflags

e9da0a6

Enable PUGIXML_STRING_VIEW when building tests

d7c5eee

dantargz pushed a commit to dantargz/pugixml that referenced this pull request Oct 22, 2024

Pick up in-progress PR from brandl-muc zeux#627

afab50c

dantargz mentioned this pull request Oct 22, 2024

Pick up in-progress PR from brandl-muc dantargz/pugixml#1

Closed

zeux closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add string view support (only impl, no tests) #627

Add string view support (only impl, no tests) #627

brandl-muc commented Sep 10, 2024

zeux Sep 11, 2024

brandl-muc Sep 13, 2024

zeux Sep 13, 2024 •

edited

Loading

zeux Sep 11, 2024

brandl-muc Sep 13, 2024 •

edited

Loading

zeux Sep 13, 2024

zeux commented Sep 11, 2024

brandl-muc commented Sep 13, 2024 •

edited

Loading

zeux commented Sep 13, 2024 •

edited

Loading

brandl-muc commented Sep 13, 2024

zeux commented Sep 16, 2024

brandl-muc commented Sep 17, 2024

dantargz commented Oct 22, 2024

brandl-muc commented Oct 22, 2024

dantargz commented Oct 22, 2024

zeux commented Oct 22, 2024

Add string view support (only impl, no tests) #627

Add string view support (only impl, no tests) #627

Conversation

brandl-muc commented Sep 10, 2024

zeux Sep 11, 2024

Choose a reason for hiding this comment

brandl-muc Sep 13, 2024

Choose a reason for hiding this comment

zeux Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

zeux Sep 11, 2024

Choose a reason for hiding this comment

brandl-muc Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

zeux Sep 13, 2024

Choose a reason for hiding this comment

zeux commented Sep 11, 2024

brandl-muc commented Sep 13, 2024 • edited Loading

zeux commented Sep 13, 2024 • edited Loading

brandl-muc commented Sep 13, 2024

zeux commented Sep 16, 2024

brandl-muc commented Sep 17, 2024

dantargz commented Oct 22, 2024

brandl-muc commented Oct 22, 2024

dantargz commented Oct 22, 2024

zeux commented Oct 22, 2024

zeux Sep 13, 2024 •

edited

Loading

brandl-muc Sep 13, 2024 •

edited

Loading

brandl-muc commented Sep 13, 2024 •

edited

Loading

zeux commented Sep 13, 2024 •

edited

Loading