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

[Tomi Valkeinen]

On 29/08/2022 17:16, Laurent Pinchart wrote:
> Hi Tomi,
> 
> 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.

  Tomi