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

[Laurent Pinchart]

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/

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

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

Laurent Pinchart