Leverage std::string_view overloads in pugixml 1.15

[myd7349]

Remove unnecessary c_str() calls and rely on the new std::string overloads introduced in pugixml 1.15.

Close #247

[cboulay]

Thanks!
Don't worry about those Apple CI failures (I have a separate fix for that).
Please try to fix the ubuntu failures.

[myd7349]

Thanks! Don't worry about those Apple CI failures (I have a separate fix for that). Please try to fix the ubuntu failures.

Hi! @cboulay Thank you for your review. I am trying to fix the CI failures.

[myd7349]

I have fixed the compilation error on Ubuntu.

Specifically, this error occurs when -DLSL_BUNDLED_PUGIXML=OFF is specified:

liblsl/.github/workflows/cppcmake.yml

Line 43 in 63741a9

- {name: "ubuntu-24.04-x64", os: "ubuntu-24.04", arch: "x86_64", cmake_extra: "-DLSL_BUNDLED_PUGIXML=OFF" }

pugixml 1.15 added std::string overloads (in fact, std::string_view) for some APIs, but older versions of pugixml do not support them. When liblsl is built against a non-bundled pugixml prior to 1.15, these issues arise. Therefore, to use the new overloads while remaining compatible with older pugixml versions, it is necessary to check whether the PUGIXML_HAS_STRING_VIEW macro is defined.

https://github.com/zeux/pugixml/blob/v1.15/src/pugixml.hpp#L445-L447

[cboulay]

We can see in #254 that the Apple actions runners pass. The issue is either due to rebasing on some recent change I made or because only my commits can access the secrets. Either way, this PR will pass the tests.

[Copilot]

Pull request overview

This PR updates the codebase to leverage the new std::string_view overloads introduced in pugixml 1.15. The changes remove unnecessary c_str() calls when passing std::string values to pugixml methods, using conditional compilation to maintain backward compatibility with older versions.

Changes:

• Added conditional compilation blocks using PUGIXML_HAS_STRING_VIEW to distinguish between new and legacy pugixml versions
• Removed redundant c_str() calls when the new string_view overloads are available
• Updated multiple setter functions and a template specialization to use the cleaner API

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The code has significant duplication across multiple functions with identical conditional compilation patterns. Consider creating a wrapper function or macro to eliminate this repetition and improve maintainability.

Suggested change

	template <> void append_text_node(xml_node &node, const char *name, const std::string &value) {
	#if defined(PUGIXML_HAS_STRING_VIEW)
	node.append_child(name).append_child(node_pcdata).set_value(value);
	#else
	node.append_child(name).append_child(node_pcdata).set_value(value.c_str());
	#endif
	}
	namespace {
	inline void set_node_value_from_string(xml_node &node, const std::string &value) {
	#if defined(PUGIXML_HAS_STRING_VIEW)
	node.set_value(value);
	#else
	node.set_value(value.c_str());
	#endif
	}
	}
	template <> void append_text_node(xml_node &node, const char *name, const std::string &value) {
	xml_node child = node.append_child(name).append_child(node_pcdata);
	set_node_value_from_string(child, value);
	}

[cboulay]

Thanks for this PR!

I think the copilot bot missed something...
Since liblsl requires C++17 (see cmake/CompilerSettings.cmake), PUGIXML_HAS_STRING_VIEW will always be defined; see manual here.
Could you simplify this to just remove the .c_str() calls unconditionally without the #ifdef guards?

(Or we could just use #254 where I made this change.)

[cboulay]

Sorry, I'm multitasking (debugging some noise issues in the lab) and completely missed / forgot about your comment:

When liblsl is built against a non-bundled pugixml prior to 1.15, these issues arise.

I'll see if I can update the dependency and/or not rely on system pugixml. I'll need a bit of time to look into that.

[cboulay]

OK we can leave the #ifdef in here for now.
I'm considering moving away from using system pugixml entirely, and relying on cmake fetch content to pull it in. I do this in another project and it works well.

However, that requires some more configuration -- static linking? make sure we hide all the pugixml symbols to not conflict, etc. It's well beyond the scope of this PR.

I'm satisfied with this as is. I'll merge it in this evening or tomorrow.

[myd7349]

Hi！ @cboulay Thank you for your review.

Currently, the versions of pugixml available across various Linux distributions and package managers.

Once pugixml 1.15 is everywhere, we can remove the #if defined(PUGIXML_HAS_STRING_VIEW).

[myd7349]

BTW, this is the packaging status of liblsl:

[cboulay]

That's pretty cool! How did you generate those images?

1.17 came out like a week ago so I expect it will take some time to propagate to these other distribution channels (that I have no control over).

[myd7349]

https://repology.org/

[![Packaging status](https://repology.org/badge/vertical-allrepos/pugixml.svg?columns=3)](https://repology.org/project/pugixml/versions)

[![Packaging status](https://repology.org/badge/vertical-allrepos/liblsl.svg?columns=1)](https://repology.org/project/liblsl/versions)

[myd7349]

Hi! @cboulay

Do you like the idea of adding a packaging status badge to liblsl? Here is a preview of how it would look:

https://github.com/myd7349/liblsl/tree/update-readme

This would make it easy for maintainers and users to see which package managers provide liblsl and which versions are available. The badge would also be updated automatically.