[libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix LogCategory creation issues

[Laurent Pinchart]

Hi Tomi,

On Mon, Aug 29, 2022 at 05:27:57PM +0300, Tomi Valkeinen wrote:
> On 29/08/2022 17:16, Laurent Pinchart wrote:
> > On Mon, Aug 29, 2022 at 11:18:29AM +0300, Tomi Valkeinen wrote:
> >> On 27/08/2022 04:27, Laurent Pinchart wrote:
> >>> On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote:
> >>>> Each declaration of a LogCategory will create a new LogCategory, and
> >>>> will be stored in an unordered_set Logger::categories_. This means that
> >>>> when a plugin .so is unloaded and loaded, as happens when destructing
> >>>> and creating a CamereManager, we'll get duplicate categories.
> >>>>
> >>>> The Logger::registerCategory docs say "Log categories must have unique
> >>>> names. If a category with the same name already exists this function
> >>>> performs no operation.". The code does not comply with this.
> >>>>
> >>>> We solve the issue with two changes:
> >>>>
> >>>> Change the unordered_set to a vector for simplicity, as there's no need
> >>>> for an unordered_set.
> >>>>
> >>>> Instead of using the LogCategory constructor to create new categories in
> >>>> _LOG_CATEGORY() macro, use a factory method. The factory method will
> >>>> return either an existing LogCategory if one exists with the given name,
> >>>> or a newly created one.
> >>>>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >>>> ---
> >>>>    include/libcamera/base/log.h |  6 +++--
> >>>>    src/libcamera/base/log.cpp   | 51 +++++++++++++++++++++++++++++++-----
> >>>>    2 files changed, 48 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> >>>> index 8b462767..76e01118 100644
> >>>> --- a/include/libcamera/base/log.h
> >>>> +++ b/include/libcamera/base/log.h
> >>>> @@ -29,7 +29,7 @@ enum LogSeverity {
> >>>>    class LogCategory
> >>>>    {
> >>>>    public:
> >>>> -	explicit LogCategory(const char *name);
> >>>> +	static LogCategory *create(const std::string &name);
> >>>
> >>> Let's pass a const char * to this function, to have a single
> >>> construction of a std::string inside the function instead of in every
> >>> caller.
> >>
> >> Well, I think we should never use char * in C++ code unless absolutely
> >> necessary as we have a proper class for string, which makes it much
> >> harder to write buggy code wrt. string operations. Here, I think that's
> >> a bit of a pointless optimization considering the amount of times these
> >> functions are called.
> >>
> >> Nevertheless, a better option compared to std::string (and, I think,
> >> char *) would be std::string_view. Is that fine for you?
> > 
> > It would, but I see you've kept const char * in v2 after falling into
> > the rabbit hole of comparing performances :-) We don't use
> 
> Yes, and with string's small-strings-optimization it gets "interesting"...
> 
> > std::string_view in libcamera, but maybe we should consider it, I like
> > the idea of bundling the string pointer and size together, much like we
> > do in the Span class.
> 
> Yes, I haven't used string_view very much yet, but I think I like it a 
> lot. Especially cases where you want to, e.g., pass a substring to a 
> function, and instead of creating a new string you just pass a 
> string_view which points to the original one. Or, say, splitting a 
> string into tokens, with string_views no new strings are created.

Indeed. StringSplitter::iterator (in include/libcamera/base/utils.h)
would be a good candidate for our first use of std::string_view.

-- 
Regards,

Laurent Pinchart