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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran