util/xmlfile.h: API update #14032
util/xmlfile.h: API update #14032
Conversation
ajrhacker
commented
Aug 8, 2025
ajrhacker
commented
- Convert most const char * parameters to std::string_view, cleaning up associated use of null pointers
- Rename get_name and get_value methods to name and value and make them return references to std::string
- Convert most const char * parameters to std::string_view, cleaning up associated use of null pointers - Rename get_name and get_value methods to name and value and make them return references to std::string
| // return the string value of an attribute, or the specified default if not present | ||
| const char *get_attribute_string(const char *attribute, const char *defvalue) const; | ||
| // return the string value of an attribute, or the specified default or empty string if not present | ||
| std::string_view get_attribute_string(std::string_view attribute, std::string_view defvalue = std::string_view()) const; |
There was a problem hiding this comment.
I really dislike treating not present as an empty string by default. It goes against the “XML way”. Maybe you could make it return a std::optional<std::string_view> instead? Then the default value could be std::nullopt.
| char const *get_value() const { return m_value.empty() ? nullptr : m_value.c_str(); } | ||
| void set_value(char const *value); | ||
| void append_value(char const *value, int length); | ||
| const std::string &value() const { return m_value; } | ||
| void set_value(std::string_view value) { m_value.assign(value); } | ||
| void append_value(std::string_view value) { m_value.append(value); } |
There was a problem hiding this comment.
Exposing that the value is a std::string in the getter feels like a bit of a leaky abstraction. I know you’re doing it (rather than returning std::string_view) for convenience because various things want to use the “value” for things that want a NUL-terminated string, and you can use .c_str() this way, but it feels dirtier before because it changes it from “value is internally stored as something that can be exposed as a NUL-terminated stringto “value is internally stored as astd::string`”.
