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

11923 diff mbox series

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/log.h | 15 ++++++++-------
 src/libcamera/log.cpp            | 17 ++++++++---------
 2 files changed, 16 insertions(+), 16 deletions(-)

Hey Laurent,

Thank you for the patch.

On 14.04.2021 00: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, I have tested the patch on my machine and I can confirm
that there is no visible change.

>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net>

>---
> 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()
>
> /*
>  * 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
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel

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@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@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(),
>

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@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@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(),

On 14/04/2021 10:40, Laurent Pinchart wrote:
> 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@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.

Ah yes, I missed that, and I think that's a valuable feature too.

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

In thinking the macros weren't needed, I thought that would be handled
by a separate function overload that would call through with a null
category.

But the Category shortening alone is a churn I wouldn't want to lose so
I don't think there's much to change here.

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

Still stands ... ;-)


>>>  /*
>>>   * 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(),
>