[libcamera-devel] [PATCH v2 02/11] libcamera: log: Add an ASSERT macro
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jan 8 12:03:24 CET 2019
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
More information about the libcamera-devel
mailing list