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

[Laurent Pinchart]

On Thu, Feb 06, 2025 at 05:47:59PM +0000, Barnabás Pőcze wrote:
> 2025. február 6., csütörtök 18:16 keltezéssel, Laurent Pinchart írta:
> > 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.

I haven't found this being documented in
https://en.cppreference.com/w/cpp/container/list but I agree that's how
the libstdc++ and libc++ implementations behave.

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

It probably won't make much of a difference indeed.

I'm OK with this patch, but I think you'll need to call
categories_.clear() explicitly in the destructor with the mutex locked,
or TSA should complain.

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