[RFC PATCH v2 9/9] libcamera: base: log: Store categories in list

[Laurent Pinchart]

Hi Barnabás,

Thank you for the patch.

On Mon, Feb 03, 2025 at 05:59:39PM +0000, Barnabás Pőcze wrote:
> Store the `LogCategory` objects in an `std::list`. This eliminates
> the need for manually deleting them in the destructor while guaranteeing
> address stability.
> 
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
>  src/libcamera/base/log.cpp | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 69aa16c4c..003e2cf3a 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -407,7 +407,7 @@ private:
>  	static bool destroyed_;
>  
>  	Mutex mutex_;
> -	std::vector<LogCategory *> categories_;
> +	std::list<LogCategory> categories_;

Will this cause two allocations, one for the list node, and one for the
LogCategory instance ?

Iterating over a vector should also be (slightly) more efficient than
iterating over a list. What is the gola of this patch ? If it is only to
avoid manual deletion, we can also store unique pointers in the vector.

>  	std::list<std::pair<std::string, LogSeverity>> levels_;
>  
>  	std::shared_ptr<LogOutput> output_;
> @@ -524,9 +524,6 @@ void logSetLevel(const char *category, const char *level)
>  Logger::~Logger()
>  {
>  	destroyed_ = true;
> -
> -	for (LogCategory *category : categories_)
> -		delete category;
>  }
>  
>  /**
> @@ -659,9 +656,9 @@ void Logger::logSetLevel(const char *category, const char *level)
>  
>  	MutexLocker locker(mutex_);
>  
> -	for (LogCategory *c : categories_) {
> -		if (c->name() == category) {
> -			c->setSeverity(severity);
> +	for (LogCategory &c : categories_) {
> +		if (c.name() == category) {
> +			c.setSeverity(severity);
>  			break;
>  		}
>  	}
> @@ -717,12 +714,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name)
>  {
>  	MutexLocker locker(mutex_);
>  
> -	for (LogCategory *category : categories_) {
> -		if (category->name() == name)
> -			return category;
> +	for (LogCategory &category : categories_) {
> +		if (category.name() == name)
> +			return &category;
>  	}
>  
> -	LogCategory *category = categories_.emplace_back(new LogCategory(name));
> +	LogCategory &category = categories_.emplace_back(name);
>  
>  	for (const std::pair<std::string, LogSeverity> &level : levels_) {
>  		bool match = true;
> @@ -739,12 +736,12 @@ LogCategory *Logger::findOrCreateCategory(std::string_view name)
>  		}
>  
>  		if (match) {
> -			category->setSeverity(level.second);
> +			category.setSeverity(level.second);
>  			break;
>  		}
>  	}
>  
> -	return category;
> +	return &category;
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart