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

[Barnabás Pőcze]

Hi


2025. február 6., csütörtök 18:16 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> 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 ?

With std::list there will be a single allocation for each category.


> 
> 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.

To avoid manual deletion. A vector was another option that I have considered,
but in the end I went with a list because it's less code and the types look cleaner.
I'm not sure if there is much difference between std::list<T> and
std::vector<std::unique_ptr<T>> since both options kind of require one indirection
when going to the next element.


> 
> >  	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
>