[v2,2/5] libcamera: base: log: Do not instantiate disabled `LogMessage`s

25847 diff mbox series

At the moment every `LOG()` macro invocation results in a `LogMessage` being
created, the message serialized into an `std::stringstream`. Only in the
destructor is it actually checked whether the given `LogCategory` enables
the given log level.

This is not too efficient, it would be better to skip the log message
construction and all the `operator<<()` invocations if the message will
just be discarded.

This could be easily done if the `LOG()` macro accepted its arguments like a
traditional function as in that case an appropriate `if` statement can be
injected in a do-while loop. However, that is not the case, the `LOG()` macro
should effectively "return" a stream.

It is not possible inject an `if` statement directly as that would
lead to issues:

  if (...)
    LOG(...)
  else
   ...

The `else` would bind the to the `if` in the `LOG()` macro. This is
diagnosed by `-Wdangling-else`.

An alternative approach would be to use a `for` loop and force a single
iteration using a boolean flag or similar. This is entirely doable but
I think the implemented approach is easier to understand.

This change implements the early log level checking using a `switch` statement
as this avoids the dangling else related issues. One small issue arises
because having a boolean controlling expression is diagnosed by clang
(`-Wswitch-bool`); the result is cast to `int` to avoid the warning.

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

Quoting Barnabás Pőcze (2026-01-19 11:31:00)
> At the moment every `LOG()` macro invocation results in a `LogMessage` being
> created, the message serialized into an `std::stringstream`. Only in the
> destructor is it actually checked whether the given `LogCategory` enables
> the given log level.
> 
> This is not too efficient, it would be better to skip the log message
> construction and all the `operator<<()` invocations if the message will
> just be discarded.
> 
> This could be easily done if the `LOG()` macro accepted its arguments like a
> traditional function as in that case an appropriate `if` statement can be
> injected in a do-while loop. However, that is not the case, the `LOG()` macro
> should effectively "return" a stream.
> 
> It is not possible inject an `if` statement directly as that would
> lead to issues:
> 
>   if (...)
>     LOG(...)
>   else
>    ...
> 
> The `else` would bind the to the `if` in the `LOG()` macro. This is
> diagnosed by `-Wdangling-else`.
> 
> An alternative approach would be to use a `for` loop and force a single
> iteration using a boolean flag or similar. This is entirely doable but
> I think the implemented approach is easier to understand.
> 
> This change implements the early log level checking using a `switch` statement
> as this avoids the dangling else related issues. One small issue arises
> because having a boolean controlling expression is diagnosed by clang
> (`-Wswitch-bool`); the result is cast to `int` to avoid the warning.

I can live with that.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/base/log.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> index d8338d8df..dbd14951c 100644
> --- a/include/libcamera/base/log.h
> +++ b/include/libcamera/base/log.h
> @@ -108,10 +108,15 @@ LogMessage _log(const LogCategory &category, LogSeverity severity,
>  #ifndef __DOXYGEN__
>  #define _LOG_CATEGORY(name) logCategory##name
>  
> +#define _LOG(cat, sev) \
> +       switch (const auto &_logCategory = (cat); int(_logCategory.severity() <= Log##sev)) \
> +       case 1: \
> +               _log(_logCategory, Log##sev).stream()
> +
>  #define _LOG1(severity) \
> -       _log(LogCategory::defaultCategory(), Log##severity).stream()
> +       _LOG(LogCategory::defaultCategory(), severity)
>  #define _LOG2(category, severity) \
> -       _log(_LOG_CATEGORY(category)(), Log##severity).stream()
> +       _LOG(_LOG_CATEGORY(category)(), severity)
>  
>  /*
>   * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of
> -- 
> 2.52.0
>