[v1,1/2] libcamera: base: log: Remove log level check for "Fatal" messages
diff mbox series

Message ID 20260507135019.231615-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1,1/2] libcamera: base: log: Remove log level check for "Fatal" messages
Related show

Commit Message

Barnabás Pőcze May 7, 2026, 1:50 p.m. UTC 
When a fatal log message is used, it is expected that it will be displayed
and will in some way abort the execution. Since the log level of each
message is known at compile time, the log level check can be short-circuited
if a fatal message is intended.

This also essentially reverts 2318a2863baa ("libcamera: base: log: Inline `LOG()` into `ASSERT()`")
as there is no need to inline the `_log()` calls to avoid the runtime condition.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/base/log.h | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

Comments

Laurent Pinchart May 7, 2026, 2:05 p.m. UTC | #1 
Hi Barnabás,

Thank you for the patch.

On Thu, May 07, 2026 at 03:50:18PM +0200, Barnabás Pőcze wrote:
> When a fatal log message is used, it is expected that it will be displayed
> and will in some way abort the execution. Since the log level of each
> message is known at compile time, the log level check can be short-circuited
> if a fatal message is intended.
> 
> This also essentially reverts 2318a2863baa ("libcamera: base: log: Inline `LOG()` into `ASSERT()`")
> as there is no need to inline the `_log()` calls to avoid the runtime condition.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/base/log.h | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> index 0751387a4..5cb14311e 100644
> --- a/include/libcamera/base/log.h
> +++ b/include/libcamera/base/log.h
> @@ -108,10 +108,22 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,
>  #ifndef __DOXYGEN__
>  #define _LOG_CATEGORY(name) logCategory##name
>  
> -#define _LOG(cat, sev)                                                 \
> -	switch (const auto &_logCategory = (cat);                      \
> -		static_cast<int>(_logCategory.severity() <= Log##sev)) \
> -	case 1:                                                        \
> +/* Returns `int` to avoid `-Wswitch-bool` below. */
> +template<LogSeverity Severity>
> +constexpr int isLogSeverityEnabled(const LogCategory &category)
> +{
> +	static_assert(LogDebug <= Severity && Severity <= LogFatal);
> +
> +	if constexpr (Severity < LogFatal)
> +		return static_cast<unsigned int>(category.severity()) <= Severity;
> +	else
> +		return true;
> +}
> +
> +#define _LOG(cat, sev)                                        \
> +	switch (const auto &_logCategory = (cat);             \
> +		isLogSeverityEnabled<Log##sev>(_logCategory)) \
> +	case 1:                                               \
>  		_log(_logCategory, Log##sev).stream()
>  
>  #define _LOG1(severity) \
> @@ -130,11 +142,11 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,
>  #endif /* __DOXYGEN__ */
>  
>  #ifndef NDEBUG
> -#define ASSERT(condition) static_cast<void>(({                          \
> -	if (!(condition))                                               \
> -		_log(LogCategory::defaultCategory(), LogFatal).stream() \
> -			<< "assertion \"" #condition "\" failed in "    \
> -			<< __func__ << "()";                            \
> +#define ASSERT(condition) static_cast<void>(({                       \
> +	if (!(condition))                                            \
> +		LOG(Fatal)                                           \
> +			<< "assertion \"" #condition "\" failed in " \
> +			<< __func__ << "()";                         \
>  }))
>  #else
>  #define ASSERT(condition) static_cast<void>(false && (condition))
Barnabás Pőcze May 7, 2026, 2:25 p.m. UTC | #2 
2026. 05. 07. 16:05 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for the patch.

If my mail client is not lying, then there is no content after this.
Was that intentional?


> 
> On Thu, May 07, 2026 at 03:50:18PM +0200, Barnabás Pőcze wrote:
>> When a fatal log message is used, it is expected that it will be displayed
>> and will in some way abort the execution. Since the log level of each
>> message is known at compile time, the log level check can be short-circuited
>> if a fatal message is intended.
>>
>> This also essentially reverts 2318a2863baa ("libcamera: base: log: Inline `LOG()` into `ASSERT()`")
>> as there is no need to inline the `_log()` calls to avoid the runtime condition.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/base/log.h | 30 +++++++++++++++++++++---------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
>> index 0751387a4..5cb14311e 100644
>> --- a/include/libcamera/base/log.h
>> +++ b/include/libcamera/base/log.h
>> @@ -108,10 +108,22 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,
>>   #ifndef __DOXYGEN__
>>   #define _LOG_CATEGORY(name) logCategory##name
>>   
>> -#define _LOG(cat, sev)                                                 \
>> -	switch (const auto &_logCategory = (cat);                      \
>> -		static_cast<int>(_logCategory.severity() <= Log##sev)) \
>> -	case 1:                                                        \
>> +/* Returns `int` to avoid `-Wswitch-bool` below. */
>> +template<LogSeverity Severity>
>> +constexpr int isLogSeverityEnabled(const LogCategory &category)
>> +{
>> +	static_assert(LogDebug <= Severity && Severity <= LogFatal);
>> +
>> +	if constexpr (Severity < LogFatal)
>> +		return static_cast<unsigned int>(category.severity()) <= Severity;
>> +	else
>> +		return true;
>> +}
>> +
>> +#define _LOG(cat, sev)                                        \
>> +	switch (const auto &_logCategory = (cat);             \
>> +		isLogSeverityEnabled<Log##sev>(_logCategory)) \
>> +	case 1:                                               \
>>   		_log(_logCategory, Log##sev).stream()
>>   
>>   #define _LOG1(severity) \
>> @@ -130,11 +142,11 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,
>>   #endif /* __DOXYGEN__ */
>>   
>>   #ifndef NDEBUG
>> -#define ASSERT(condition) static_cast<void>(({                          \
>> -	if (!(condition))                                               \
>> -		_log(LogCategory::defaultCategory(), LogFatal).stream() \
>> -			<< "assertion \"" #condition "\" failed in "    \
>> -			<< __func__ << "()";                            \
>> +#define ASSERT(condition) static_cast<void>(({                       \
>> +	if (!(condition))                                            \
>> +		LOG(Fatal)                                           \
>> +			<< "assertion \"" #condition "\" failed in " \
>> +			<< __func__ << "()";                         \
>>   }))
>>   #else
>>   #define ASSERT(condition) static_cast<void>(false && (condition))
>
Laurent Pinchart May 7, 2026, 2:31 p.m. UTC | #3 
On Thu, May 07, 2026 at 04:25:16PM +0200, Barnabás Pőcze wrote:
> 2026. 05. 07. 16:05 keltezéssel, Laurent Pinchart írta:
> > Hi Barnabás,
> > 
> > Thank you for the patch.
> 
> If my mail client is not lying, then there is no content after this.
> Was that intentional?

Not at all. I think I was so mesmerized by the code that I forgot to add

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > On Thu, May 07, 2026 at 03:50:18PM +0200, Barnabás Pőcze wrote:
> >> When a fatal log message is used, it is expected that it will be displayed
> >> and will in some way abort the execution. Since the log level of each
> >> message is known at compile time, the log level check can be short-circuited
> >> if a fatal message is intended.
> >>
> >> This also essentially reverts 2318a2863baa ("libcamera: base: log: Inline `LOG()` into `ASSERT()`")
> >> as there is no need to inline the `_log()` calls to avoid the runtime condition.
> >>
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   include/libcamera/base/log.h | 30 +++++++++++++++++++++---------
> >>   1 file changed, 21 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> >> index 0751387a4..5cb14311e 100644
> >> --- a/include/libcamera/base/log.h
> >> +++ b/include/libcamera/base/log.h
> >> @@ -108,10 +108,22 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,
> >>   #ifndef __DOXYGEN__
> >>   #define _LOG_CATEGORY(name) logCategory##name
> >>   
> >> -#define _LOG(cat, sev)                                                 \
> >> -	switch (const auto &_logCategory = (cat);                      \
> >> -		static_cast<int>(_logCategory.severity() <= Log##sev)) \
> >> -	case 1:                                                        \
> >> +/* Returns `int` to avoid `-Wswitch-bool` below. */
> >> +template<LogSeverity Severity>
> >> +constexpr int isLogSeverityEnabled(const LogCategory &category)
> >> +{
> >> +	static_assert(LogDebug <= Severity && Severity <= LogFatal);
> >> +
> >> +	if constexpr (Severity < LogFatal)
> >> +		return static_cast<unsigned int>(category.severity()) <= Severity;
> >> +	else
> >> +		return true;
> >> +}
> >> +
> >> +#define _LOG(cat, sev)                                        \
> >> +	switch (const auto &_logCategory = (cat);             \
> >> +		isLogSeverityEnabled<Log##sev>(_logCategory)) \
> >> +	case 1:                                               \
> >>   		_log(_logCategory, Log##sev).stream()
> >>   
> >>   #define _LOG1(severity) \
> >> @@ -130,11 +142,11 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,
> >>   #endif /* __DOXYGEN__ */
> >>   
> >>   #ifndef NDEBUG
> >> -#define ASSERT(condition) static_cast<void>(({                          \
> >> -	if (!(condition))                                               \
> >> -		_log(LogCategory::defaultCategory(), LogFatal).stream() \
> >> -			<< "assertion \"" #condition "\" failed in "    \
> >> -			<< __func__ << "()";                            \
> >> +#define ASSERT(condition) static_cast<void>(({                       \
> >> +	if (!(condition))                                            \
> >> +		LOG(Fatal)                                           \
> >> +			<< "assertion \"" #condition "\" failed in " \
> >> +			<< __func__ << "()";                         \
> >>   }))
> >>   #else
> >>   #define ASSERT(condition) static_cast<void>(false && (condition))
> >

Patch
diff mbox series 

diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
index 0751387a4..5cb14311e 100644
--- a/include/libcamera/base/log.h
+++ b/include/libcamera/base/log.h
@@ -108,10 +108,22 @@  LogMessage _log(const LogCategory &category, LogSeverity severity,
 #ifndef __DOXYGEN__
 #define _LOG_CATEGORY(name) logCategory##name
 
-#define _LOG(cat, sev)                                                 \
-	switch (const auto &_logCategory = (cat);                      \
-		static_cast<int>(_logCategory.severity() <= Log##sev)) \
-	case 1:                                                        \
+/* Returns `int` to avoid `-Wswitch-bool` below. */
+template<LogSeverity Severity>
+constexpr int isLogSeverityEnabled(const LogCategory &category)
+{
+	static_assert(LogDebug <= Severity && Severity <= LogFatal);
+
+	if constexpr (Severity < LogFatal)
+		return static_cast<unsigned int>(category.severity()) <= Severity;
+	else
+		return true;
+}
+
+#define _LOG(cat, sev)                                        \
+	switch (const auto &_logCategory = (cat);             \
+		isLogSeverityEnabled<Log##sev>(_logCategory)) \
+	case 1:                                               \
 		_log(_logCategory, Log##sev).stream()
 
 #define _LOG1(severity) \
@@ -130,11 +142,11 @@  LogMessage _log(const LogCategory &category, LogSeverity severity,
 #endif /* __DOXYGEN__ */
 
 #ifndef NDEBUG
-#define ASSERT(condition) static_cast<void>(({                          \
-	if (!(condition))                                               \
-		_log(LogCategory::defaultCategory(), LogFatal).stream() \
-			<< "assertion \"" #condition "\" failed in "    \
-			<< __func__ << "()";                            \
+#define ASSERT(condition) static_cast<void>(({                       \
+	if (!(condition))                                            \
+		LOG(Fatal)                                           \
+			<< "assertion \"" #condition "\" failed in " \
+			<< __func__ << "()";                         \
 }))
 #else
 #define ASSERT(condition) static_cast<void>(false && (condition))