[libcamera-devel] [PATCH 2/2] libcamera: log: Use compiler builtins to retrieve file and line number

[Kieran Bingham]

Hi Laurent,

On 13/04/2021 22:51, Laurent Pinchart wrote:
> Replace the __FILE__ and __LINE__ values passed to the _log() function
> with default parameters, taking their values from the __builtin_FILE()
> and __builtin_LINE() functions. This moves handling of the file and line
> from the preprocessor to the compiler, which is generally preferred as
> it increases type safety.
> 

Sounds good to me.


> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/internal/log.h | 15 ++++++++-------
>  src/libcamera/log.cpp            | 17 ++++++++---------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/include/libcamera/internal/log.h b/include/libcamera/internal/log.h
> index 0fdacc4733fe..be0bab3c1272 100644
> --- a/include/libcamera/internal/log.h
> +++ b/include/libcamera/internal/log.h
> @@ -89,21 +89,22 @@ public:
>  protected:
>  	virtual std::string logPrefix() const = 0;
>  
> -	LogMessage _log(const char *file, unsigned int line,
> -			const LogCategory *category,
> -			LogSeverity severity) const;
> +	LogMessage _log(const LogCategory *category, LogSeverity severity,
> +			const char *fileName = __builtin_FILE(),
> +			unsigned int line = __builtin_LINE()) const;
>  };
>  
> -LogMessage _log(const char *file, unsigned int line,
> -		const LogCategory *category, LogSeverity severity);
> +LogMessage _log(const LogCategory *category, LogSeverity severity,
> +		const char *fileName = __builtin_FILE(),
> +		unsigned int line = __builtin_LINE());
>  
>  #ifndef __DOXYGEN__
>  #define _LOG_CATEGORY(name) logCategory##name
>  
>  #define _LOG1(severity) \
> -	_log(__FILE__, __LINE__, nullptr, Log##severity).stream()
> +	_log(nullptr, Log##severity).stream()
>  #define _LOG2(category, severity) \
> -	_log(__FILE__, __LINE__, &_LOG_CATEGORY(category)(), Log##severity).stream()
> +	_log(&_LOG_CATEGORY(category)(), Log##severity).stream()

Do we actually need these macros any more at all?

>From what I can see they serve the following now:

 - Make LOG all caps in the code
   I think this is actually helpful to make log lines clear

 - Adds the .stream() call after the construction
   Maybe this is helpful. Can it be done with a direct call by
   overriding the << operator or something directly though to make the
   object itself look like a stream?


Otherwise,

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


>  
>  /*
>   * Expand the LOG() macro to _LOG1() or _LOG2() based on the number of
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 9f86e645ac58..94175ab34535 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -897,19 +897,18 @@ Loggable::~Loggable()
>  
>  /**
>   * \brief Create a temporary LogMessage object to log a message
> - * \param[in] fileName The file name where the message is logged from
> - * \param[in] line The line number where the message is logged from
>   * \param[in] category The log message category
>   * \param[in] severity The log message severity
> + * \param[in] fileName The file name where the message is logged from
> + * \param[in] line The line number where the message is logged from
>   *
>   * This method is used as a backeng by the LOG() macro to create a log message
>   * for locations inheriting from the Loggable class.
>   *
>   * \return A log message
>   */
> -LogMessage Loggable::_log(const char *fileName, unsigned int line,
> -			  const LogCategory *category,
> -			  LogSeverity severity) const
> +LogMessage Loggable::_log(const LogCategory *category, LogSeverity severity,
> +			  const char *fileName, unsigned int line) const
>  {
>  	LogMessage msg(fileName, line,
>  		       category ? *category : LogCategory::defaultCategory(),
> @@ -921,18 +920,18 @@ LogMessage Loggable::_log(const char *fileName, unsigned int line,
>  
>  /**
>   * \brief Create a temporary LogMessage object to log a message
> - * \param[in] fileName The file name where the message is logged from
> - * \param[in] line The line number where the message is logged from
>   * \param[in] category The log message category
>   * \param[in] severity The log message severity
> + * \param[in] fileName The file name where the message is logged from
> + * \param[in] line The line number where the message is logged from
>   *
>   * This function is used as a backeng by the LOG() macro to create a log
>   * message for locations not inheriting from the Loggable class.
>   *
>   * \return A log message
>   */
> -LogMessage _log(const char *fileName, unsigned int line,
> -		const LogCategory *category, LogSeverity severity)
> +LogMessage _log(const LogCategory *category, LogSeverity severity,
> +		const char *fileName, unsigned int line)
>  {
>  	return LogMessage(fileName, line,
>  			  category ? *category : LogCategory::defaultCategory(),
> 

-- 
Regards
--
Kieran