Leverage std::string_view overloads in pugixml 1.15 by myd7349 · Pull Request #253 · sccn/liblsl

myd7349 · 2026-01-17T03:04:27Z

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

Close #247

cboulay · 2026-01-17T03:27:33Z

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

myd7349 · 2026-01-17T03:49:18Z

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.

Close sccn#247

myd7349 · 2026-01-17T05:05:10Z

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 · 2026-01-17T18:31:35Z

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 · 2026-01-17T18:34:20Z

src/stream_info_impl.cpp

 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
 }



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 · 2026-01-17T18:40:56Z

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 · 2026-01-17T18:50:53Z

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 · 2026-01-17T19:53:12Z

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 · 2026-01-18T02:46:18Z

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 · 2026-01-18T03:01:17Z

BTW, this is the packaging status of liblsl:

cboulay · 2026-01-18T03:59:17Z

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 · 2026-01-18T04:09:12Z

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 · 2026-01-18T04:54:58Z

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.

Leverage std::string overloads in pugixml 1.15

8a214f1

Close sccn#247

myd7349 force-pushed the fix-247 branch from 1bf3b11 to 8a214f1 Compare January 17, 2026 04:55

myd7349 marked this pull request as ready for review January 17, 2026 05:05

myd7349 changed the title ~~Leverage std::string overloads in pugixml 1.15~~ Leverage std::string_view overloads in pugixml 1.15 Jan 17, 2026

cboulay mentioned this pull request Jan 17, 2026

Leverage std::string overloads in pugixml 1.15 #254

Closed

cboulay requested a review from Copilot January 17, 2026 18:33

Copilot AI reviewed Jan 17, 2026

View reviewed changes

cboulay mentioned this pull request Jan 18, 2026

Pugixml fetch only #255

Merged

cboulay merged commit 6df1fd8 into sccn:dev Jan 19, 2026
12 of 18 checks passed

myd7349 deleted the fix-247 branch January 20, 2026 03:50

Conversation

myd7349 commented Jan 17, 2026

Uh oh!

cboulay commented Jan 17, 2026

Uh oh!

myd7349 commented Jan 17, 2026

Uh oh!

myd7349 commented Jan 17, 2026

Uh oh!

cboulay commented Jan 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

cboulay commented Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cboulay commented Jan 17, 2026

Uh oh!

cboulay commented Jan 17, 2026

Uh oh!

myd7349 commented Jan 18, 2026

Uh oh!

myd7349 commented Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cboulay commented Jan 18, 2026

Uh oh!

myd7349 commented Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myd7349 commented Jan 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cboulay commented Jan 17, 2026 •

edited

Loading

myd7349 commented Jan 18, 2026 •

edited

Loading

myd7349 commented Jan 18, 2026 •

edited

Loading