[libcamera-devel] [PATCH v2 02/11] libcamera: log: Add an ASSERT macro

[Kieran Bingham]

Hi Laurent,

On 08/01/2019 10:59, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tuesday, 8 January 2019 12:34:50 EET Kieran Bingham wrote:
>> On 07/01/2019 23:11, Laurent Pinchart wrote:
>>> The ASSERT() macro is similar to the assert() macro defined by the C
>>> standard, but uses the libcamera logging infrastructure.
>>
>> Only discussion on this patch below - no blocker.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>> ---
>>> Changes since v1:
>>>
>>> - Fix syntax error in ASSERT() macro
>>> - Fix NDEBUG conditional compilation
>>> - Fix typo in documentation
>>> - Fix ASSERT() condition check
>>> ---
>>>
>>>  src/libcamera/include/log.h |  9 +++++++++
>>>  src/libcamera/log.cpp       | 16 ++++++++++++++++
>>>  2 files changed, 25 insertions(+)
>>>
>>> diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h
>>> index 03842be02d0e..c1af3741ffee 100644
>>> --- a/src/libcamera/include/log.h
>>> +++ b/src/libcamera/include/log.h
>>>
>>> @@ -36,6 +36,15 @@ private:
>>>  #define LOG(severity) LogMessage(__FILE__, __LINE__,
>>>  Log##severity).stream()> 
>>> +#ifndef NDEBUG
>>
>> I'm interested in your choice of negating the DEBUG symbol here.
>>    (i.e. instead of using "#ifdef DEBUG")
> 
> It's how the POSIX, C and C++ standards handle assert():
> 
> http://man7.org/linux/man-pages/man3/assert.3.html
> http://www.cplusplus.com/reference/cassert/assert/

Aha yes - of course ... that's a good reason :)


>> Would you expect us to keep ASSERTs always on by default (i.e. for users
>> of the library?) or would you foresee a release defining NDEBUG.
> 
> I foresee a release defining NDEBUG.
> 
>> I guess one benefit of negating this - means that by default anyone self
>> compiling the library (and thus developing) gets debug assertions
>> enabled, while a release binary can specify the NDEBUG as part of any
>> automated build process if necessary.
> 
> I don't know what the rationale behind the NDEBUG vs. DEBUG choice was, but 
> this would be a good one :-)
> 
>> Would the LogDebug level also be compiled out if NDEBUG is defined?
>>  (not relevant for this patch, just curious as to your opinion)
> 
> I'm tempted to keep it, as it's useful to debug applications using libcamera 
> in addition to debugging libcamera itself.

Yes, we'll have different log levels provided at runtime anyway so it's
all filterable.

Clears up my thoughts anyway, thanks.

--
Kieran





>>> +#define ASSERT(condition) static_cast<void>(({				\
>>> +	if (!(condition))							\
>>> +		LOG(Fatal) << "assertion \"" #condition "\" failed";	\
>>> +}))
>>> +#else
>>> +#define ASSERT(condition) static_cast<void>(false && (condition))
>>> +#endif
>>> +
>>>  } /* namespace libcamera */
>>>  
>>>  #endif /* __LIBCAMERA_LOG_H__ */
>>> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>>> index a5823c64eaa6..6785d371449e 100644
>>> --- a/src/libcamera/log.cpp
>>> +++ b/src/libcamera/log.cpp
>>> @@ -48,6 +48,22 @@ namespace libcamera {
>>>
>>>   * terminates immediately after printing the message.
>>>   */
>>>
>>> +/**
>>> + * \def ASSERT(condition)
>>> + * \brief Abort program execution if assertion fails
>>> + *
>>> + * If \a condition is false, ASSERT() logs an error message with the
>>> Fatal log + * level and aborts program execution.
>>> + *
>>> + * If the macro NDEBUG is defined before including log.h, ASSERT()
>>> generates no + * code.
>>> + *
>>> + * Using conditions that have side effects with ASSERT() is not
>>> recommended, as + * these effects would depend on whether NDEBUG is
>>> defined or not. Similarly, + * ASSERT() should not be used to check for
>>> errors that can occur under normal + * conditions as those checks would
>>> then be removed when compiling with NDEBUG. + */
>>> +
>>>
>>>  static const char *log_severity_name(LogSeverity severity)
>>>  {
>>>  
>>>  	static const char * const names[] = {
> 
> 

-- 
Regards
--
Kieran