Skip to content

Improved support for string_views for TOML and XML #445

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.

Sign up for GitHub

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

Merged
liuzicheng1987 merged 2 commits into from
Jun 8, 2025
Merged

Improved support for string_views for TOML and XML #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b9a252
Improved support for string_views for TOML and XML
liuzicheng1987 Jun 8, 2025
b18d2fe
Upgrade to pugixml 1.15 in order to be able to use string views
liuzicheng1987 Jun 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def requirements(self):
if self.options.with_toml:
self.requires("tomlplusplus/3.4.0", transitive_headers=True)
if self.options.with_xml:
self.requires("pugixml/1.14", transitive_headers=True)
self.requires("pugixml/1.15", transitive_headers=True)
if self.options.with_yaml:
self.requires("yaml-cpp/0.8.0", transitive_headers=True)

Expand Down
11 changes: 3 additions & 8 deletions include/rfl/toml/read.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include <istream>
#include <string>
#include <string_view>
#include <toml++/toml.hpp>

#include "../Processors.hpp"
Expand All @@ -25,18 +26,12 @@ auto read(InputVarType _var) {
return Parser<T, ProcessorsType>::read(r, _var);
}

/// Reads a TOML string.
template <class T, class... Ps>
Result<internal::wrap_in_rfl_array_t<T>> read(const std::string& _toml_str) {
auto table = ::toml::parse(_toml_str);
return read<T, Ps...>(&table);
}

/// Reads a TOML string.
template <class T, class... Ps>
Result<internal::wrap_in_rfl_array_t<T>> read(
const std::string_view _toml_str) {
return read<T, Ps...>(std::string(_toml_str));
auto table = ::toml::parse(_toml_str);
return read<T, Ps...>(&table);
Comment on lines +33 to +34
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This change to directly use ::toml::parse(_toml_str) with std::string_view (and subsequently removing the std::string overload that converted std::string_view to std::string) is a good efficiency improvement.

Previously, a std::string_view might be converted to a std::string before parsing (as seen in the removed overload return read<T, Ps...>(std::string(_toml_str));), which involved an unnecessary allocation and copy.
By parsing the std::string_view directly using toml::parse (which supports std::string_view), this overhead is avoided. This is a valuable optimization, especially for performance-sensitive scenarios. Well done!

}

/// Parses an object from a stringstream.
Expand Down
8 changes: 3 additions & 5 deletions include/rfl/xml/read.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@
#include "Parser.hpp"
#include "Reader.hpp"

namespace rfl {
namespace xml {
namespace rfl ::xml {

using InputVarType = typename Reader::InputVarType;

Expand All @@ -32,7 +31,7 @@ auto read(const InputVarType& _var) {
template <class T, class... Ps>
Result<T> read(const std::string_view _xml_str) {
pugi::xml_document doc;
const auto result = doc.load_string(_xml_str.data());
const auto result = doc.load_buffer(_xml_str.data(), _xml_str.size());
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Excellent change switching from doc.load_string(_xml_str.data()) to doc.load_buffer(_xml_str.data(), _xml_str.size())!

Using load_string with _xml_str.data() could be unsafe because std::string_view::data() doesn't guarantee a null-terminated string. If the underlying character sequence isn't null-terminated, load_string might read past the end of the buffer, leading to undefined behavior or security vulnerabilities.

load_buffer, by taking an explicit size, correctly and safely handles std::string_view data, regardless of null termination. This significantly improves the robustness and security of XML parsing from a std::string_view. Great catch and important fix!

if (!result) {
return error("XML string could not be parsed: " +
std::string(result.description()));
Expand All @@ -49,7 +48,6 @@ auto read(std::istream& _stream) {
return read<T, Ps...>(xml_str);
}

} // namespace xml
} // namespace rfl
} // namespace rfl::xml

#endif
20 changes: 10 additions & 10 deletions src/rfl/xml/Writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ Writer::OutputArrayType Writer::add_array_to_object(

Writer::OutputVarType Writer::add_value_to_array_impl(
const std::string& _str, OutputArrayType* _parent) const noexcept {
auto node_child = Ref<pugi::xml_node>::make(
_parent->node_->append_child(_parent->name_.data()));
auto node_child =
Ref<pugi::xml_node>::make(_parent->node_->append_child(_parent->name_));
node_child->append_child(pugi::node_pcdata).set_value(_str.c_str());
return OutputVarType(node_child);
}
Expand All @@ -88,38 +88,38 @@ Writer::OutputVarType Writer::add_value_to_object_impl(
const std::string_view& _name, const std::string& _str,
OutputObjectType* _parent, const bool _is_attribute) const noexcept {
if (_is_attribute) {
_parent->node_->append_attribute(_name.data()) = _str.c_str();
_parent->node_->append_attribute(_name) = _str.c_str();
return OutputVarType(_parent->node_);
} else if (_name == XML_CONTENT) {
_parent->node_->append_child(pugi::node_pcdata).set_value(_str.c_str());
return OutputVarType(_parent->node_);
} else {
auto node_child =
Ref<pugi::xml_node>::make(_parent->node_->append_child(_name.data()));
Ref<pugi::xml_node>::make(_parent->node_->append_child(_name));
node_child->append_child(pugi::node_pcdata).set_value(_str.c_str());
return OutputVarType(node_child);
}
}

Writer::OutputObjectType Writer::add_object_to_array(
const size_t _size, OutputArrayType* _parent) const noexcept {
auto node_child = Ref<pugi::xml_node>::make(
_parent->node_->append_child(_parent->name_.data()));
auto node_child =
Ref<pugi::xml_node>::make(_parent->node_->append_child(_parent->name_));
return OutputObjectType(node_child);
}

Writer::OutputObjectType Writer::add_object_to_object(
const std::string_view& _name, const size_t _size,
OutputObjectType* _parent) const noexcept {
auto node_child =
Ref<pugi::xml_node>::make(_parent->node_->append_child(_name.data()));
Ref<pugi::xml_node>::make(_parent->node_->append_child(_name));
return OutputObjectType(node_child);
}

Writer::OutputVarType Writer::add_null_to_array(
OutputArrayType* _parent) const noexcept {
auto node_child = Ref<pugi::xml_node>::make(
_parent->node_->append_child(_parent->name_.data()));
auto node_child =
Ref<pugi::xml_node>::make(_parent->node_->append_child(_parent->name_));
return OutputVarType(node_child);
}

Expand All @@ -132,7 +132,7 @@ Writer::OutputVarType Writer::add_null_to_object(
return OutputVarType(_parent->node_);
} else {
auto node_child =
Ref<pugi::xml_node>::make(_parent->node_->append_child(_name.data()));
Ref<pugi::xml_node>::make(_parent->node_->append_child(_name));
return OutputVarType(node_child);
}
}
Expand Down
2 changes: 1 addition & 1 deletion vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
"dependencies": [
{
"name": "pugixml",
"version>=": "1.14"
"version>=": "1.15"
}
]
},
Expand Down
Loading