[{"id":24801,"web_url":"https://patchwork.libcamera.org/comment/24801/","msgid":"","date":"2022-08-27T01:27:32","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote:\n> Each declaration of a LogCategory will create a new LogCategory, and\n> will be stored in an unordered_set Logger::categories_. This means that\n> when a plugin .so is unloaded and loaded, as happens when destructing\n> and creating a CamereManager, we'll get duplicate categories.\n> \n> The Logger::registerCategory docs say \"Log categories must have unique\n> names. If a category with the same name already exists this function\n> performs no operation.\". The code does not comply with this.\n> \n> We solve the issue with two changes:\n> \n> Change the unordered_set to a vector for simplicity, as there's no need\n> for an unordered_set.\n> \n> Instead of using the LogCategory constructor to create new categories in\n> _LOG_CATEGORY() macro, use a factory method. The factory method will\n> return either an existing LogCategory if one exists with the given name,\n> or a newly created one.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> include/libcamera/base/log.h | 6 +++--\n> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++-----\n> 2 files changed, 48 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> index 8b462767..76e01118 100644\n> --- a/include/libcamera/base/log.h\n> +++ b/include/libcamera/base/log.h\n> @@ -29,7 +29,7 @@ enum LogSeverity {\n> class LogCategory\n> {\n> public:\n> -\texplicit LogCategory(const char *name);\n> +\tstatic LogCategory *create(const std::string &name);\n\nLet's pass a const char * to this function, to have a single\nconstruction of a std::string inside the function instead of in every\ncaller.\n\n> \n> \tconst std::string &name() const { return name_; }\n> \tLogSeverity severity() const { return severity_; }\n> @@ -38,6 +38,8 @@ public:\n> \tstatic const LogCategory &defaultCategory();\n> \n> private:\n> +\texplicit LogCategory(const std::string &name);\n\nIs the change from const char * to std::string needed here ? If so, does\nit belong to 1/2 ?\n\n> +\n> \tconst std::string name_;\n> \tLogSeverity severity_;\n> };\n> @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)();\n> const LogCategory &_LOG_CATEGORY(name)()\t\t\t\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> \t/* The instance will be deleted by the Logger destructor. */\t\\\n> -\tstatic LogCategory *category = new LogCategory(#name);\t\t\\\n> +\tstatic LogCategory *category = LogCategory::create(#name);\t\\\n> \treturn *category;\t\t\t\t\t\t\\\n> }\n> \n> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> index a4a5b452..c6500e96 100644\n> --- a/src/libcamera/base/log.cpp\n> +++ b/src/libcamera/base/log.cpp\n> @@ -314,10 +314,11 @@ private:\n> \n> \tfriend LogCategory;\n> \tvoid registerCategory(LogCategory *category);\n> +\tLogCategory *findCategory(const std::string &name) const;\n> \n> \tstatic bool destroyed_;\n> \n> -\tstd::unordered_set categories_;\n> +\tstd::vector categories_;\n> \tstd::list> levels_;\n> \n> \tstd::shared_ptr output_;\n> @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level)\n> * \\brief Register a log category with the logger\n> * \\param[in] category The log category\n> *\n> - * Log categories must have unique names. If a category with the same name\n> - * already exists this function performs no operation.\n> + * Log categories must have unique names. It is illegal to call this function\n\nNobody will arrest you for doing so, so you can write invalid instead of\nillegal :-)\n\n> + * if a log category with the same name already exists.\n> */\n> void Logger::registerCategory(LogCategory *category)\n> {\n> -\tcategories_.insert(category);\n> +\tASSERT(!findCategory(category->name()));\n\nI wonder if this is overkill, as the caller checks that the category\nisn't found first. I suppose the performance penalty of the lookup is\nacceptable in debug mode, but maybe it's not needed ?\n\n> +\n> +\tcategories_.push_back(category);\n\nThis reminds me we need to add locking to the logger. That's out of\nscope for this patch of course.\n\n> \n> \tconst std::string &name = category->name();\n> \tfor (const std::pair &level : levels_) {\n> @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category)\n> \t}\n> }\n> \n> +/**\n> + * \\brief Find an existing log category with the given name\n> + * \\param[in] name The name of the log category\n\n * \\return ...\n\n> + */\n> +LogCategory *Logger::findCategory(const std::string &name) const\n> +{\n> +\tif (auto it = std::find_if(categories_.begin(), categories_.end(),\n> +\t\t\t\t [&name](auto c) { return name == c->name(); });\n\nWould it be more efficient to store categories in an unordered_map\nindexed by the category name ?\n\n> +\t it != categories_.end()) {\n> +\t\treturn *it;\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> /**\n> * \\enum LogSeverity\n> * Log message severity\n> @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category)\n> * and is used to control the log level per group.\n> */\n> \n> +/**\n> + * \\brief Create a new LogCategory or return an existing one\n\n * \\param[in] name ...\n\n> + * \\return The pointer to the LogCategory\n\nThe return documentation goes to the end of the comment.\n\n> + *\n> + * Create and return a new LogCategory with the given name if such a category\n> + * does not yet exist, or return the existing one.\n> + */\n> +LogCategory *LogCategory::create(const std::string &name)\n> +{\n> +\tLogCategory *category = Logger::instance()->findCategory(name);\n> +\n> +\tif (!category) {\n> +\t\tcategory = new LogCategory(name);\n> +\n\nYou could drop the blank line.\n\n> +\t\tLogger::instance()->registerCategory(category);\n> +\t}\n> +\n> +\treturn category;\n> +}\n> +\n> /**\n> * \\brief Construct a log category\n> * \\param[in] name The category name\n> */\n> -LogCategory::LogCategory(const char *name)\n> +LogCategory::LogCategory(const std::string &name)\n> \t: name_(name), severity_(LogSeverity::LogInfo)\n> {\n> -\tLogger::instance()->registerCategory(this);\n> }\n> \n> /**\n> @@ -804,7 +841,7 @@ void LogCategory::setSeverity(LogSeverity severity)\n> */\n> const LogCategory &LogCategory::defaultCategory()\n> {\n> -\tstatic const LogCategory *category = new LogCategory(\"default\");\n> +\tstatic const LogCategory *category = LogCategory::create(\"default\");\n> \treturn *category;\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E3CA9C3272\n\tfor ;\n\tSat, 27 Aug 2022 01:27:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C15D61FC0;\n\tSat, 27 Aug 2022 03:27:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48B61603E1\n\tfor ;\n\tSat, 27 Aug 2022 03:27:40 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C6044A8;\n\tSat, 27 Aug 2022 03:27:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661563662;\n\tbh=oyZQ3UnXP1S8QoJdCJFCxqLUIvd77VeLfTjQLxD76bE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=S3u3mkFKF+WNQrFLVQsTWalpw7LLtxWLVeM5/rO+1tOR+/aMsv7c2/yQJTCAh7u+7\n\taTddYxkyiFg3FfUBZin8uxiw05EKr94UKO9BvLdSfEfnPhaZN25RRMDe56h+7CCiNF\n\t6Ym8jFU3HKkZ9H8l8d0ZMjvhCHSvsNq6Dq784bT+FuXrykR7cQ7OD3d5MZEBA7W8h4\n\tzkmqkCDba7SxrsnYHJFQE2HpS4EQ3GTIQFE7GDtWq2RhyYtWOKWzieYUMUJgpvN25s\n\tzlvSVSXzHYSmYdFk719I9mrtACMzjzBTCsUIfqxaSnU4oD3wGIwDsCjAMbvr8pQyl2\n\tMOHR4E4nvQ1ZQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661563659;\n\tbh=oyZQ3UnXP1S8QoJdCJFCxqLUIvd77VeLfTjQLxD76bE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dYs5gP6EcgKt93sfIn0fzqr+JlwxoJVaFNjhS8JVq4jDz4iCjtnrjMeaHhdiZ5ho9\n\tCs9BekaxskhS1klr844O4LDRoyODih/SOKmGFSNq85Zb0lHazWJJVFl1z0Z3VPIZuD\n\tzQhmAz3Sm/Py7L/UiluNkJAs65NAvgOBkYjbThqY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dYs5gP6E\"; dkim-atps=neutral","Date":"Sat, 27 Aug 2022 04:27:32 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220826113931.59323-1-tomi.valkeinen@ideasonboard.com>\n\t<20220826113931.59323-3-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220826113931.59323-3-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24812,"web_url":"https://patchwork.libcamera.org/comment/24812/","msgid":"<1e4ec969-d52c-7187-8a39-6b771ef77b18@ideasonboard.com>","date":"2022-08-29T08:18:29","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 27/08/2022 04:27, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote:\n>> Each declaration of a LogCategory will create a new LogCategory, and\n>> will be stored in an unordered_set Logger::categories_. This means that\n>> when a plugin .so is unloaded and loaded, as happens when destructing\n>> and creating a CamereManager, we'll get duplicate categories.\n>>\n>> The Logger::registerCategory docs say \"Log categories must have unique\n>> names. If a category with the same name already exists this function\n>> performs no operation.\". The code does not comply with this.\n>>\n>> We solve the issue with two changes:\n>>\n>> Change the unordered_set to a vector for simplicity, as there's no need\n>> for an unordered_set.\n>>\n>> Instead of using the LogCategory constructor to create new categories in\n>> _LOG_CATEGORY() macro, use a factory method. The factory method will\n>> return either an existing LogCategory if one exists with the given name,\n>> or a newly created one.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> include/libcamera/base/log.h | 6 +++--\n>> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++-----\n>> 2 files changed, 48 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n>> index 8b462767..76e01118 100644\n>> --- a/include/libcamera/base/log.h\n>> +++ b/include/libcamera/base/log.h\n>> @@ -29,7 +29,7 @@ enum LogSeverity {\n>> class LogCategory\n>> {\n>> public:\n>> -\texplicit LogCategory(const char *name);\n>> +\tstatic LogCategory *create(const std::string &name);\n> \n> Let's pass a const char * to this function, to have a single\n> construction of a std::string inside the function instead of in every\n> caller.\n\nWell, I think we should never use char * in C++ code unless absolutely \nnecessary as we have a proper class for string, which makes it much \nharder to write buggy code wrt. string operations. Here, I think that's \na bit of a pointless optimization considering the amount of times these \nfunctions are called.\n\nNevertheless, a better option compared to std::string (and, I think, \nchar *) would be std::string_view. Is that fine for you?\n\n>> \n>> \tconst std::string &name() const { return name_; }\n>> \tLogSeverity severity() const { return severity_; }\n>> @@ -38,6 +38,8 @@ public:\n>> \tstatic const LogCategory &defaultCategory();\n>> \n>> private:\n>> +\texplicit LogCategory(const std::string &name);\n> \n> Is the change from const char * to std::string needed here ? If so, does\n> it belong to 1/2 ?\n\nI don't think the change from char * to string is needed in either \npatch. It just made sense to me to get rid of the char * use. It could \nbe dropped or done before either patch.\n\n>> +\n>> \tconst std::string name_;\n>> \tLogSeverity severity_;\n>> };\n>> @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)();\n>> const LogCategory &_LOG_CATEGORY(name)()\t\t\t\t\\\n>> {\t\t\t\t\t\t\t\t\t\\\n>> \t/* The instance will be deleted by the Logger destructor. */\t\\\n>> -\tstatic LogCategory *category = new LogCategory(#name);\t\t\\\n>> +\tstatic LogCategory *category = LogCategory::create(#name);\t\\\n>> \treturn *category;\t\t\t\t\t\t\\\n>> }\n>> \n>> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n>> index a4a5b452..c6500e96 100644\n>> --- a/src/libcamera/base/log.cpp\n>> +++ b/src/libcamera/base/log.cpp\n>> @@ -314,10 +314,11 @@ private:\n>> \n>> \tfriend LogCategory;\n>> \tvoid registerCategory(LogCategory *category);\n>> +\tLogCategory *findCategory(const std::string &name) const;\n>> \n>> \tstatic bool destroyed_;\n>> \n>> -\tstd::unordered_set categories_;\n>> +\tstd::vector categories_;\n>> \tstd::list> levels_;\n>> \n>> \tstd::shared_ptr output_;\n>> @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level)\n>> * \\brief Register a log category with the logger\n>> * \\param[in] category The log category\n>> *\n>> - * Log categories must have unique names. If a category with the same name\n>> - * already exists this function performs no operation.\n>> + * Log categories must have unique names. It is illegal to call this function\n> \n> Nobody will arrest you for doing so, so you can write invalid instead of\n> illegal :-)\n\nAre you sure? ... Well, ok...\n\n>> + * if a log category with the same name already exists.\n>> */\n>> void Logger::registerCategory(LogCategory *category)\n>> {\n>> -\tcategories_.insert(category);\n>> +\tASSERT(!findCategory(category->name()));\n> \n> I wonder if this is overkill, as the caller checks that the category\n> isn't found first. I suppose the performance penalty of the lookup is\n> acceptable in debug mode, but maybe it's not needed ?\n\nI can drop this. The function is called only inside this file, so I \nthink we can expect the parameter to be correct.\n\n>> +\n>> +\tcategories_.push_back(category);\n> \n> This reminds me we need to add locking to the logger. That's out of\n> scope for this patch of course.\n> \n>> \n>> \tconst std::string &name = category->name();\n>> \tfor (const std::pair &level : levels_) {\n>> @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category)\n>> \t}\n>> }\n>> \n>> +/**\n>> + * \\brief Find an existing log category with the given name\n>> + * \\param[in] name The name of the log category\n> \n> * \\return ...\n\nOk.\n\n>> + */\n>> +LogCategory *Logger::findCategory(const std::string &name) const\n>> +{\n>> +\tif (auto it = std::find_if(categories_.begin(), categories_.end(),\n>> +\t\t\t\t [&name](auto c) { return name == c->name(); });\n> \n> Would it be more efficient to store categories in an unordered_map\n> indexed by the category name ?\n\nDepends what you mean with efficient... =)\n\nI think we'll usually have some tens of entries. My pure guess is that \nthe memory use with unordered_map would be much larger than with a \nvector, and we would be constructing string instances for the keys. The \nlookup difference would probably be unnoticeable with these amount of \nentries. The for loops we have that iterate over the categories_ would \nprobably be slower with an unordered_map.\n\nAnd considering we'll call this function a few tens of times, I again \nthink trying to optimize this is an unnecessary optimization.\n\nI think a more valid question is if an unordered_map or map would suit \nbetter code-wise (readability, maintainability) for this case. With the \ncurrent uses of 'categories_', I don't see a big difference either way. \nUsing a map would simplify this lookup, but iterating with for would be \na bit more complex.\n\n>> +\t it != categories_.end()) {\n>> +\t\treturn *it;\n>> +\t}\n>> +\n>> +\treturn nullptr;\n>> +}\n>> +\n>> /**\n>> * \\enum LogSeverity\n>> * Log message severity\n>> @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category)\n>> * and is used to control the log level per group.\n>> */\n>> \n>> +/**\n>> + * \\brief Create a new LogCategory or return an existing one\n> \n> * \\param[in] name ...\n\nOk.\n\n>> + * \\return The pointer to the LogCategory\n> \n> The return documentation goes to the end of the comment.\n\nOk.\n\n>> + *\n>> + * Create and return a new LogCategory with the given name if such a category\n>> + * does not yet exist, or return the existing one.\n>> + */\n>> +LogCategory *LogCategory::create(const std::string &name)\n>> +{\n>> +\tLogCategory *category = Logger::instance()->findCategory(name);\n>> +\n>> +\tif (!category) {\n>> +\t\tcategory = new LogCategory(name);\n>> +\n> \n> You could drop the blank line.\n\nOk.\n\n Tomi","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4B65AC3272\n\tfor ;\n\tMon, 29 Aug 2022 08:18:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9894661FC0;\n\tMon, 29 Aug 2022 10:18:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 820A361F9F\n\tfor ;\n\tMon, 29 Aug 2022 10:18:32 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EA475481;\n\tMon, 29 Aug 2022 10:18:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661761114;\n\tbh=CI9lZO3LqiyerjrE5dvEkV01EHDzEGuMXm4ymDbB/HE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=F5IBbnnkwVLzZfMZZmUbqG6xgH+pSTMWML0aznZ1pRKiQBtaB2yqSytbSpTRba/OJ\n\t4P318KJYcJqZA2Pemkyix1yeQpXQtVYOLdkcIeuq1YSMQ/u8g9kAvlsBuxvRX+ee2k\n\tAaO1OLQ8vcfuUUHiNbP9bux9pUP3stbrdQs1aVBFXtqqvg4l4qECye+IrNl0FQTzer\n\tPm/lZ26e5STfX4POnXBA6asC9+4Efm2eozSeCwFbEr79tmmhKYq1y7bNTyXYTrbOBy\n\tXC4JYHwCX/72o1Z/xUSJEjovvF4vU0mwpSGt5Jl0/X+KbKFBG+NObJ+Az/CkpNz/eD\n\tVdll05cUE6V6Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661761112;\n\tbh=CI9lZO3LqiyerjrE5dvEkV01EHDzEGuMXm4ymDbB/HE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=dXz5hbmIat44FBxgxwjRLVC+69NpOLXleAZRcTjUYwwai+havPuF/TjSVw37mo8wy\n\t+sgnGaP/YM2KlPsCpDMXM0uTd3/O9G3nuvKWWWwgsUHzXH1SS2jF1XFqPW8VMLIygc\n\thpfS5BAW2Ea60Bm7BHMVayhKgaNelCtYqnAdmb3I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dXz5hbmI\"; dkim-atps=neutral","Message-ID":"<1e4ec969-d52c-7187-8a39-6b771ef77b18@ideasonboard.com>","Date":"Mon, 29 Aug 2022 11:18:29 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220826113931.59323-1-tomi.valkeinen@ideasonboard.com>\n\t<20220826113931.59323-3-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24814,"web_url":"https://patchwork.libcamera.org/comment/24814/","msgid":"","date":"2022-08-29T14:16:54","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Mon, Aug 29, 2022 at 11:18:29AM +0300, Tomi Valkeinen wrote:\n> On 27/08/2022 04:27, Laurent Pinchart wrote:\n> > On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote:\n> >> Each declaration of a LogCategory will create a new LogCategory, and\n> >> will be stored in an unordered_set Logger::categories_. This means that\n> >> when a plugin .so is unloaded and loaded, as happens when destructing\n> >> and creating a CamereManager, we'll get duplicate categories.\n> >>\n> >> The Logger::registerCategory docs say \"Log categories must have unique\n> >> names. If a category with the same name already exists this function\n> >> performs no operation.\". The code does not comply with this.\n> >>\n> >> We solve the issue with two changes:\n> >>\n> >> Change the unordered_set to a vector for simplicity, as there's no need\n> >> for an unordered_set.\n> >>\n> >> Instead of using the LogCategory constructor to create new categories in\n> >> _LOG_CATEGORY() macro, use a factory method. The factory method will\n> >> return either an existing LogCategory if one exists with the given name,\n> >> or a newly created one.\n> >>\n> >> Signed-off-by: Tomi Valkeinen \n> >> ---\n> >> include/libcamera/base/log.h | 6 +++--\n> >> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++-----\n> >> 2 files changed, 48 insertions(+), 9 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> >> index 8b462767..76e01118 100644\n> >> --- a/include/libcamera/base/log.h\n> >> +++ b/include/libcamera/base/log.h\n> >> @@ -29,7 +29,7 @@ enum LogSeverity {\n> >> class LogCategory\n> >> {\n> >> public:\n> >> -\texplicit LogCategory(const char *name);\n> >> +\tstatic LogCategory *create(const std::string &name);\n> > \n> > Let's pass a const char * to this function, to have a single\n> > construction of a std::string inside the function instead of in every\n> > caller.\n> \n> Well, I think we should never use char * in C++ code unless absolutely \n> necessary as we have a proper class for string, which makes it much \n> harder to write buggy code wrt. string operations. Here, I think that's \n> a bit of a pointless optimization considering the amount of times these \n> functions are called.\n> \n> Nevertheless, a better option compared to std::string (and, I think, \n> char *) would be std::string_view. Is that fine for you?\n\nIt would, but I see you've kept const char * in v2 after falling into\nthe rabbit hole of comparing performances :-) We don't use\nstd::string_view in libcamera, but maybe we should consider it, I like\nthe idea of bundling the string pointer and size together, much like we\ndo in the Span class.\n\n> >> \n> >> \tconst std::string &name() const { return name_; }\n> >> \tLogSeverity severity() const { return severity_; }\n> >> @@ -38,6 +38,8 @@ public:\n> >> \tstatic const LogCategory &defaultCategory();\n> >> \n> >> private:\n> >> +\texplicit LogCategory(const std::string &name);\n> > \n> > Is the change from const char * to std::string needed here ? If so, does\n> > it belong to 1/2 ?\n> \n> I don't think the change from char * to string is needed in either \n> patch. It just made sense to me to get rid of the char * use. It could \n> be dropped or done before either patch.\n> \n> >> +\n> >> \tconst std::string name_;\n> >> \tLogSeverity severity_;\n> >> };\n> >> @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)();\n> >> const LogCategory &_LOG_CATEGORY(name)()\t\t\t\t\\\n> >> {\t\t\t\t\t\t\t\t\t\\\n> >> \t/* The instance will be deleted by the Logger destructor. */\t\\\n> >> -\tstatic LogCategory *category = new LogCategory(#name);\t\t\\\n> >> +\tstatic LogCategory *category = LogCategory::create(#name);\t\\\n> >> \treturn *category;\t\t\t\t\t\t\\\n> >> }\n> >> \n> >> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> >> index a4a5b452..c6500e96 100644\n> >> --- a/src/libcamera/base/log.cpp\n> >> +++ b/src/libcamera/base/log.cpp\n> >> @@ -314,10 +314,11 @@ private:\n> >> \n> >> \tfriend LogCategory;\n> >> \tvoid registerCategory(LogCategory *category);\n> >> +\tLogCategory *findCategory(const std::string &name) const;\n> >> \n> >> \tstatic bool destroyed_;\n> >> \n> >> -\tstd::unordered_set categories_;\n> >> +\tstd::vector categories_;\n> >> \tstd::list> levels_;\n> >> \n> >> \tstd::shared_ptr output_;\n> >> @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level)\n> >> * \\brief Register a log category with the logger\n> >> * \\param[in] category The log category\n> >> *\n> >> - * Log categories must have unique names. If a category with the same name\n> >> - * already exists this function performs no operation.\n> >> + * Log categories must have unique names. It is illegal to call this function\n> > \n> > Nobody will arrest you for doing so, so you can write invalid instead of\n> > illegal :-)\n> \n> Are you sure? ... Well, ok...\n> \n> >> + * if a log category with the same name already exists.\n> >> */\n> >> void Logger::registerCategory(LogCategory *category)\n> >> {\n> >> -\tcategories_.insert(category);\n> >> +\tASSERT(!findCategory(category->name()));\n> > \n> > I wonder if this is overkill, as the caller checks that the category\n> > isn't found first. I suppose the performance penalty of the lookup is\n> > acceptable in debug mode, but maybe it's not needed ?\n> \n> I can drop this. The function is called only inside this file, so I \n> think we can expect the parameter to be correct.\n> \n> >> +\n> >> +\tcategories_.push_back(category);\n> > \n> > This reminds me we need to add locking to the logger. That's out of\n> > scope for this patch of course.\n> > \n> >> \n> >> \tconst std::string &name = category->name();\n> >> \tfor (const std::pair &level : levels_) {\n> >> @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category)\n> >> \t}\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Find an existing log category with the given name\n> >> + * \\param[in] name The name of the log category\n> > \n> > * \\return ...\n> \n> Ok.\n> \n> >> + */\n> >> +LogCategory *Logger::findCategory(const std::string &name) const\n> >> +{\n> >> +\tif (auto it = std::find_if(categories_.begin(), categories_.end(),\n> >> +\t\t\t\t [&name](auto c) { return name == c->name(); });\n> > \n> > Would it be more efficient to store categories in an unordered_map\n> > indexed by the category name ?\n> \n> Depends what you mean with efficient... =)\n> \n> I think we'll usually have some tens of entries. My pure guess is that \n> the memory use with unordered_map would be much larger than with a \n> vector, and we would be constructing string instances for the keys. The \n> lookup difference would probably be unnoticeable with these amount of \n> entries. The for loops we have that iterate over the categories_ would \n> probably be slower with an unordered_map.\n> \n> And considering we'll call this function a few tens of times, I again \n> think trying to optimize this is an unnecessary optimization.\n> \n> I think a more valid question is if an unordered_map or map would suit \n> better code-wise (readability, maintainability) for this case. With the \n> current uses of 'categories_', I don't see a big difference either way. \n> Using a map would simplify this lookup, but iterating with for would be \n> a bit more complex.\n\nOK.\n\n> >> +\t it != categories_.end()) {\n> >> +\t\treturn *it;\n> >> +\t}\n> >> +\n> >> +\treturn nullptr;\n> >> +}\n> >> +\n> >> /**\n> >> * \\enum LogSeverity\n> >> * Log message severity\n> >> @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category)\n> >> * and is used to control the log level per group.\n> >> */\n> >> \n> >> +/**\n> >> + * \\brief Create a new LogCategory or return an existing one\n> > \n> > * \\param[in] name ...\n> \n> Ok.\n> \n> >> + * \\return The pointer to the LogCategory\n> > \n> > The return documentation goes to the end of the comment.\n> \n> Ok.\n> \n> >> + *\n> >> + * Create and return a new LogCategory with the given name if such a category\n> >> + * does not yet exist, or return the existing one.\n> >> + */\n> >> +LogCategory *LogCategory::create(const std::string &name)\n> >> +{\n> >> +\tLogCategory *category = Logger::instance()->findCategory(name);\n> >> +\n> >> +\tif (!category) {\n> >> +\t\tcategory = new LogCategory(name);\n> >> +\n> > \n> > You could drop the blank line.\n> \n> Ok.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7D11DC0DA4\n\tfor ;\n\tMon, 29 Aug 2022 14:17:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D162861FC0;\n\tMon, 29 Aug 2022 16:17:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6CD461FBA\n\tfor ;\n\tMon, 29 Aug 2022 16:17:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02C33505;\n\tMon, 29 Aug 2022 16:17:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661782625;\n\tbh=p6483BRtIMsZzK6AeYiu6nqtbZi1mYOP5YzH5LMG+/o=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=aAIqZ0Hh7DkrF3aRM4AN2YEAR5Ngx291kgIu+rXnfZWn89uLed4ScwiHtxvfLKSKw\n\tzhqvKlHPY+imtiYwBpeYBPP+PcPFXLeF8qRxri4yKknU7Wk74SAII7f0S5uwQErpSe\n\tHYwt1YTI0Nyv3oNrjyfTopelR5OBix0yyxya3+wTkvkbL9yXYuRMpcaTKMEsn4jfPA\n\tO+Pg8jc/MBYyJOnRFMJcA0rPi9YtEIJIGe+cr97CvNKiJtOuOQCqEFxwQhJxHrZgBI\n\t3pWO6HdkQbN1p+wx65YSuDiXAmB8S1JQeCheVbKHiN1jn1/uWDORsvLu4hW/U4NvTb\n\tVZ2X9B1O0kVaw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661782624;\n\tbh=p6483BRtIMsZzK6AeYiu6nqtbZi1mYOP5YzH5LMG+/o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HLHriU69vQzKUO+wYQihoPpL/GIoQhXFpA6wFZ2PSQVf3FA3dW9GvlBAH39LaA0k+\n\tFClxzsAmLAq/1vBHWb71ajoU6LqFH9LOlPcY6LXj4b44zlP/bfNXpGpjzd5utbwUf7\n\tJZ6cPn4OzgIO+FOt6yxAcYIyRoENbzxtJ/MK47hc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HLHriU69\"; dkim-atps=neutral","Date":"Mon, 29 Aug 2022 17:16:54 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220826113931.59323-1-tomi.valkeinen@ideasonboard.com>\n\t<20220826113931.59323-3-tomi.valkeinen@ideasonboard.com>\n\t\n\t<1e4ec969-d52c-7187-8a39-6b771ef77b18@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1e4ec969-d52c-7187-8a39-6b771ef77b18@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24816,"web_url":"https://patchwork.libcamera.org/comment/24816/","msgid":"","date":"2022-08-29T14:27:57","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 29/08/2022 17:16, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Mon, Aug 29, 2022 at 11:18:29AM +0300, Tomi Valkeinen wrote:\n>> On 27/08/2022 04:27, Laurent Pinchart wrote:\n>>> On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote:\n>>>> Each declaration of a LogCategory will create a new LogCategory, and\n>>>> will be stored in an unordered_set Logger::categories_. This means that\n>>>> when a plugin .so is unloaded and loaded, as happens when destructing\n>>>> and creating a CamereManager, we'll get duplicate categories.\n>>>>\n>>>> The Logger::registerCategory docs say \"Log categories must have unique\n>>>> names. If a category with the same name already exists this function\n>>>> performs no operation.\". The code does not comply with this.\n>>>>\n>>>> We solve the issue with two changes:\n>>>>\n>>>> Change the unordered_set to a vector for simplicity, as there's no need\n>>>> for an unordered_set.\n>>>>\n>>>> Instead of using the LogCategory constructor to create new categories in\n>>>> _LOG_CATEGORY() macro, use a factory method. The factory method will\n>>>> return either an existing LogCategory if one exists with the given name,\n>>>> or a newly created one.\n>>>>\n>>>> Signed-off-by: Tomi Valkeinen \n>>>> ---\n>>>> include/libcamera/base/log.h | 6 +++--\n>>>> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++-----\n>>>> 2 files changed, 48 insertions(+), 9 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n>>>> index 8b462767..76e01118 100644\n>>>> --- a/include/libcamera/base/log.h\n>>>> +++ b/include/libcamera/base/log.h\n>>>> @@ -29,7 +29,7 @@ enum LogSeverity {\n>>>> class LogCategory\n>>>> {\n>>>> public:\n>>>> -\texplicit LogCategory(const char *name);\n>>>> +\tstatic LogCategory *create(const std::string &name);\n>>>\n>>> Let's pass a const char * to this function, to have a single\n>>> construction of a std::string inside the function instead of in every\n>>> caller.\n>>\n>> Well, I think we should never use char * in C++ code unless absolutely\n>> necessary as we have a proper class for string, which makes it much\n>> harder to write buggy code wrt. string operations. Here, I think that's\n>> a bit of a pointless optimization considering the amount of times these\n>> functions are called.\n>>\n>> Nevertheless, a better option compared to std::string (and, I think,\n>> char *) would be std::string_view. Is that fine for you?\n> \n> It would, but I see you've kept const char * in v2 after falling into\n> the rabbit hole of comparing performances :-) We don't use\n\nYes, and with string's small-strings-optimization it gets \"interesting\"...\n\n> std::string_view in libcamera, but maybe we should consider it, I like\n> the idea of bundling the string pointer and size together, much like we\n> do in the Span class.\n\nYes, I haven't used string_view very much yet, but I think I like it a \nlot. Especially cases where you want to, e.g., pass a substring to a \nfunction, and instead of creating a new string you just pass a \nstring_view which points to the original one. Or, say, splitting a \nstring into tokens, with string_views no new strings are created.\n\n Tomi","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5249FC3272\n\tfor ;\n\tMon, 29 Aug 2022 14:28:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A05D61FC0;\n\tMon, 29 Aug 2022 16:28:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5826E61FBA\n\tfor ;\n\tMon, 29 Aug 2022 16:28:01 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BFAD7505;\n\tMon, 29 Aug 2022 16:28:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661783282;\n\tbh=iP/FRyHNnAoH6XdYBpPrLMrH4nOaHyOdSlw70M/3ckA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rqgKHEwmEZG1gX+7sErnnfE9mz7Fp47qzJjwaCvBBbQHqoSB9nk8uLb800wfRSQAq\n\t44z/IUkyOxCMC4aX7CorbpmLdyKXH8dskmQxR7SHHontDPIAIqhlvvmFRqKT75Sl4h\n\t2J19ISfIJBtgMjsElgWZrLZImr+kjuGH0zTtg4D2iuktSuVFLKTcyz6nvspcdKPxmb\n\t3Y7IRYwUenFwVJBR3egpHr4Mon51rrGxgAlPwzuQQ2q8ODjvKqPA2St5uhydyKDeCB\n\t2x4jsMPVNrSZF7+gTNr7GmSaxwaB3dERcfRjTFVUEzg/T0zx5sKouLO9lP/5BEzjKD\n\tpnb3MSLlHqogw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661783280;\n\tbh=iP/FRyHNnAoH6XdYBpPrLMrH4nOaHyOdSlw70M/3ckA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=TvTq6PLFwGyeFAGKx54GB0Aqh1hcSEvpLj3+uLNBSbOvBqoE24vQEpsojfMwY5/TO\n\tRmIHAoAkXpS4peL6k9YzIKypXse9zguFluog8bM6zjSyEz/irWEsfIb3oUOpaP9u66\n\t6osXLq+zaYTNN9x4TSuQtxzfudRAvQ29Liz/8aEk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TvTq6PLF\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 29 Aug 2022 17:27:57 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220826113931.59323-1-tomi.valkeinen@ideasonboard.com>\n\t<20220826113931.59323-3-tomi.valkeinen@ideasonboard.com>\n\t\n\t<1e4ec969-d52c-7187-8a39-6b771ef77b18@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24817,"web_url":"https://patchwork.libcamera.org/comment/24817/","msgid":"","date":"2022-08-29T15:16:47","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Mon, Aug 29, 2022 at 05:27:57PM +0300, Tomi Valkeinen wrote:\n> On 29/08/2022 17:16, Laurent Pinchart wrote:\n> > On Mon, Aug 29, 2022 at 11:18:29AM +0300, Tomi Valkeinen wrote:\n> >> On 27/08/2022 04:27, Laurent Pinchart wrote:\n> >>> On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote:\n> >>>> Each declaration of a LogCategory will create a new LogCategory, and\n> >>>> will be stored in an unordered_set Logger::categories_. This means that\n> >>>> when a plugin .so is unloaded and loaded, as happens when destructing\n> >>>> and creating a CamereManager, we'll get duplicate categories.\n> >>>>\n> >>>> The Logger::registerCategory docs say \"Log categories must have unique\n> >>>> names. If a category with the same name already exists this function\n> >>>> performs no operation.\". The code does not comply with this.\n> >>>>\n> >>>> We solve the issue with two changes:\n> >>>>\n> >>>> Change the unordered_set to a vector for simplicity, as there's no need\n> >>>> for an unordered_set.\n> >>>>\n> >>>> Instead of using the LogCategory constructor to create new categories in\n> >>>> _LOG_CATEGORY() macro, use a factory method. The factory method will\n> >>>> return either an existing LogCategory if one exists with the given name,\n> >>>> or a newly created one.\n> >>>>\n> >>>> Signed-off-by: Tomi Valkeinen \n> >>>> ---\n> >>>> include/libcamera/base/log.h | 6 +++--\n> >>>> src/libcamera/base/log.cpp | 51 +++++++++++++++++++++++++++++++-----\n> >>>> 2 files changed, 48 insertions(+), 9 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> >>>> index 8b462767..76e01118 100644\n> >>>> --- a/include/libcamera/base/log.h\n> >>>> +++ b/include/libcamera/base/log.h\n> >>>> @@ -29,7 +29,7 @@ enum LogSeverity {\n> >>>> class LogCategory\n> >>>> {\n> >>>> public:\n> >>>> -\texplicit LogCategory(const char *name);\n> >>>> +\tstatic LogCategory *create(const std::string &name);\n> >>>\n> >>> Let's pass a const char * to this function, to have a single\n> >>> construction of a std::string inside the function instead of in every\n> >>> caller.\n> >>\n> >> Well, I think we should never use char * in C++ code unless absolutely\n> >> necessary as we have a proper class for string, which makes it much\n> >> harder to write buggy code wrt. string operations. Here, I think that's\n> >> a bit of a pointless optimization considering the amount of times these\n> >> functions are called.\n> >>\n> >> Nevertheless, a better option compared to std::string (and, I think,\n> >> char *) would be std::string_view. Is that fine for you?\n> > \n> > It would, but I see you've kept const char * in v2 after falling into\n> > the rabbit hole of comparing performances :-) We don't use\n> \n> Yes, and with string's small-strings-optimization it gets \"interesting\"...\n> \n> > std::string_view in libcamera, but maybe we should consider it, I like\n> > the idea of bundling the string pointer and size together, much like we\n> > do in the Span class.\n> \n> Yes, I haven't used string_view very much yet, but I think I like it a \n> lot. Especially cases where you want to, e.g., pass a substring to a \n> function, and instead of creating a new string you just pass a \n> string_view which points to the original one. Or, say, splitting a \n> string into tokens, with string_views no new strings are created.\n\nIndeed. StringSplitter::iterator (in include/libcamera/base/utils.h)\nwould be a good candidate for our first use of std::string_view.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9A711C0DA4\n\tfor ;\n\tMon, 29 Aug 2022 15:16:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D966D61FBC;\n\tMon, 29 Aug 2022 17:16:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9FE0461FBA\n\tfor ;\n\tMon, 29 Aug 2022 17:16:56 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09377505;\n\tMon, 29 Aug 2022 17:16:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661786218;\n\tbh=NQAQMspmhuXrjMdXmyIBbhgz4wOltBualdupMYI4BME=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=TajPSwTA5YkyNgs8UdfvAUAqoLALg8m/WcSD595OrVwUYJI0uo6G+vXjM7x2bVAZQ\n\tOWKP4Of4Dy3jFUILArRdz6ad8pcS/Oj/qx8dBYhFe7u18purf7xcCrwoUBrTftJnqN\n\tZWGmqc0XYXbfQRRFM5/R1k4l3yUVVAVKkvahrLZVaH3BZcPKjpbCHxTiMf89Fn1D7Z\n\tNeCTXNOaSIbq91prtWuxuI9UtBinQAY1IOx8Buw1AdzPMaP1dUcErfueC+y9EXhROm\n\tKHk8v6Dd6btbnGXVNPs+4pLc1eisqFIw8leDpcETfKaZ4eYAHCvRONP+LjDvdaMTuI\n\tDu7IXvRTqI97Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661786216;\n\tbh=NQAQMspmhuXrjMdXmyIBbhgz4wOltBualdupMYI4BME=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DfNPqAOrKLYBs/q12JN4HtG5Weyj6RzXYz4t+1ImyPT2CM55m6PCydqh/uZTv7Y6y\n\tup57T/dJKv6/yUcEfVB4cw8qL5oVHdZJ3YqnSohFmprb88l38NmpYkxqVENElpvYQF\n\tLxmx0c1u7qAOz8fUEA88T6tM28l/mxak86Ytfijk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DfNPqAOr\"; dkim-atps=neutral","Date":"Mon, 29 Aug 2022 18:16:47 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220826113931.59323-1-tomi.valkeinen@ideasonboard.com>\n\t<20220826113931.59323-3-tomi.valkeinen@ideasonboard.com>\n\t\n\t<1e4ec969-d52c-7187-8a39-6b771ef77b18@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix\n\tLogCategory creation issues","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]