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

[Kieran Bingham]

Hi Laurent,

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")

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



Would the LogDebug level also be compiled out if NDEBUG is defined?
 (not relevant for this patch, just curious as to your opinion)




> +#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