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

[Laurent Pinchart]

Hi Kieran,

On Wed, Apr 14, 2021 at 09:49:09AM +0100, Kieran Bingham wrote:
> 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?

They're also used to construct the category, and the severity. Otherwise
we'd have to write LogInfo instead of Info, but also
logCategoryV4L2Device() instead of V4L2Device.

If we wanted to simplify this, we could drop _LOG1 and _LOG2, and keep
LOG. That would make the category mandatory, we wouldn't be able to
write LOG(Info) << ... for quick debugging.

> 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,

Laurent Pinchart