[v2,14/42] libcamera: yaml_parser: Un-friend YamlParserContext from YamlObject

26454 diff mbox series

YamlParserContext is a friend of the YamlObject class to access private
member variables. Now that YamlObject exposes functions to set a value
and add children, this is not needed anymore. Decouple the two classes.

The YamlParserContext::readValue() function now takes a const reference
to the EventPtr as the YamlParserContext::parseNextYamlObject() function
needs to access it in the error handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Add error checking for add() failures
- Pass const reference to readValue() function
---
 include/libcamera/internal/yaml_parser.h |  1 -
 src/libcamera/yaml_parser.cpp            | 63 +++++++++++-------------
 2 files changed, 28 insertions(+), 36 deletions(-)

On Thu, Apr 23, 2026 at 03:59:28PM +0200, Barnabás Pőcze wrote:
> 2026. 04. 07. 17:33 keltezéssel, Laurent Pinchart írta:
> > YamlParserContext is a friend of the YamlObject class to access private
> > member variables. Now that YamlObject exposes functions to set a value
> > and add children, this is not needed anymore. Decouple the two classes.
> > 
> > The YamlParserContext::readValue() function now takes a const reference
> > to the EventPtr as the YamlParserContext::parseNextYamlObject() function
> > needs to access it in the error handler.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Add error checking for add() failures
> > - Pass const reference to readValue() function
> > ---
> >   include/libcamera/internal/yaml_parser.h |  1 -
> >   src/libcamera/yaml_parser.cpp            | 63 +++++++++++-------------
> >   2 files changed, 28 insertions(+), 36 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > index 3be61c503c88..c41397df67ab 100644
> > --- a/include/libcamera/internal/yaml_parser.h
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -205,7 +205,6 @@ private:
> >   
> >   	template<typename T>
> >   	friend struct Accessor;
> > -	friend class YamlParserContext;
> >   
> >   	enum class Type {
> >   		Dictionary,
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index 61088c7aed4f..6caacc3111c4 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -517,8 +517,8 @@ private:
> >   
> >   	EventPtr nextEvent();
> >   
> > -	void readValue(std::string &value, EventPtr event);
> > -	int parseDictionaryOrList(YamlObject::Type type,
> > +	std::string readValue(const EventPtr &event);
> > +	int parseDictionaryOrList(yaml_event_type_t endEventType,
> >   				  const std::function<int(EventPtr event)> &parseItem);
> >   	int parseNextYamlObject(YamlObject &yamlObject, EventPtr event);
> >   
> > @@ -660,22 +660,22 @@ int YamlParserContext::parseContent(YamlObject &yamlObject)
> >   /**
> >    * \fn YamlParserContext::readValue()
> >    * \brief Parse event scalar and fill its content into a string
> > - * \param[in] value The string reference to fill value
> >    *
> >    * A helper function to parse a scalar event as string. The caller needs to
> > - * guarantee the event is of scaler type.
> > + * guarantee the event is of scalar type.
> > + *
> > + * \return The scalar event value as a string
> >    */
> > -void YamlParserContext::readValue(std::string &value, EventPtr event)
> > +std::string YamlParserContext::readValue(const EventPtr &event)
> 
> I'm wondering how this came to be a non-const member function when it could
> easily be a free function in an anon namespace, or at least `static`...

Probably because it's an internal API, so it received less care. It
could be fixed on top if desired, but it shouldn't matter much as it's a
private function.

> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> >   {
> > -	value.assign(reinterpret_cast<char *>(event->data.scalar.value),
> > -		     event->data.scalar.length);
> > +	return std::string{ reinterpret_cast<const char *>(event->data.scalar.value),
> > +			    event->data.scalar.length };
> >   }
> >   
> >   /**
> >    * \fn YamlParserContext::parseDictionaryOrList()
> >    * \brief A helper function to abstract the common part of parsing dictionary or list
> > - *
> > - * \param[in] isDictionary True for parsing a dictionary, and false for a list
> > + * \param[in] endEventType The YAML end event type (sequence or mapping)
> >    * \param[in] parseItem The callback to handle an item
> >    *
> >    * A helper function to abstract parsing an item from a dictionary or a list.
> > @@ -690,13 +690,9 @@ void YamlParserContext::readValue(std::string &value, EventPtr event)
> >    * \return 0 on success or a negative error code otherwise
> >    * \retval -EINVAL The parser is failed to initialize
> >    */
> > -int YamlParserContext::parseDictionaryOrList(YamlObject::Type type,
> > +int YamlParserContext::parseDictionaryOrList(yaml_event_type_t endEventType,
> >   					     const std::function<int(EventPtr event)> &parseItem)
> >   {
> > -	yaml_event_type_t endEventType = YAML_SEQUENCE_END_EVENT;
> > -	if (type == YamlObject::Type::Dictionary)
> > -		endEventType = YAML_MAPPING_END_EVENT;
> > -
> >   	/*
> >   	 * Add a safety counter to make sure we don't loop indefinitely in case
> >   	 * the YAML file is malformed.
> > @@ -738,24 +734,19 @@ int YamlParserContext::parseNextYamlObject(YamlObject &yamlObject, EventPtr even
> >   
> >   	switch (event->type) {
> >   	case YAML_SCALAR_EVENT:
> > -		yamlObject.type_ = YamlObject::Type::Value;
> > -		readValue(yamlObject.value_, std::move(event));
> > +		yamlObject.set(readValue(event));
> >   		return 0;
> >   
> >   	case YAML_SEQUENCE_START_EVENT: {
> > -		yamlObject.type_ = YamlObject::Type::List;
> > -		auto &list = yamlObject.list_;
> > -		auto handler = [this, &list](EventPtr evt) {
> > -			list.emplace_back(std::string{}, std::make_unique<YamlObject>());
> > -			return parseNextYamlObject(*list.back().value, std::move(evt));
> > +		auto handler = [this, &yamlObject](EventPtr evt) {
> > +			YamlObject *child = yamlObject.add(std::make_unique<YamlObject>());
> > +			return parseNextYamlObject(*child, std::move(evt));
> >   		};
> > -		return parseDictionaryOrList(YamlObject::Type::List, handler);
> > +		return parseDictionaryOrList(YAML_SEQUENCE_END_EVENT, handler);
> >   	}
> >   
> >   	case YAML_MAPPING_START_EVENT: {
> > -		yamlObject.type_ = YamlObject::Type::Dictionary;
> > -		auto &list = yamlObject.list_;
> > -		auto handler = [this, &list](EventPtr evtKey) {
> > +		auto handler = [this, &yamlObject](EventPtr evtKey) {
> >   			/* Parse key */
> >   			if (evtKey->type != YAML_SCALAR_EVENT) {
> >   				LOG(YamlParser, Error) << "Expect key at line: "
> > @@ -765,26 +756,28 @@ int YamlParserContext::parseNextYamlObject(YamlObject &yamlObject, EventPtr even
> >   				return -EINVAL;
> >   			}
> >   
> > -			std::string key;
> > -			readValue(key, std::move(evtKey));
> > +			std::string key = readValue(evtKey);
> >   
> >   			/* Parse value */
> >   			EventPtr evtValue = nextEvent();
> >   			if (!evtValue)
> >   				return -EINVAL;
> >   
> > -			auto &elem = list.emplace_back(std::move(key),
> > -						       std::make_unique<YamlObject>());
> > -			return parseNextYamlObject(*elem.value, std::move(evtValue));
> > +			YamlObject *child = yamlObject.add(std::move(key),
> > +							   std::make_unique<YamlObject>());
> > +			if (!child) {
> > +				LOG(YamlParser, Error)
> > +					<< "Duplicated key at line "
> > +					<< evtKey->start_mark.line;
> > +				return -EINVAL;
> > +			}
> > +
> > +			return parseNextYamlObject(*child, std::move(evtValue));
> >   		};
> > -		int ret = parseDictionaryOrList(YamlObject::Type::Dictionary, handler);
> > +		int ret = parseDictionaryOrList(YAML_MAPPING_END_EVENT, handler);
> >   		if (ret)
> >   			return ret;
> >   
> > -		auto &dictionary = yamlObject.dictionary_;
> > -		for (const auto &elem : list)
> > -			dictionary.emplace(elem.key, elem.value.get());
> > -
> >   		return 0;
> >   	}
> >