[libcamera-devel,v2,02/11] libcamera: log: Add an ASSERT macro
diff mbox series

Message ID 20190107231151.23291-3-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Add event notification and timers
Related show

Commit Message

Laurent Pinchart Jan. 7, 2019, 11:11 p.m. UTC 
The ASSERT() macro is similar to the assert() macro defined by the C
standard, but uses the libcamera logging infrastructure.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@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(+)

Comments

Kieran Bingham Jan. 8, 2019, 10:34 a.m. UTC | #1 
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@ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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[] = {
>
Laurent Pinchart Jan. 8, 2019, 10:59 a.m. UTC | #2 
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@ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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[] = {
Kieran Bingham Jan. 8, 2019, 11:03 a.m. UTC | #3 
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@ideasonboard.com>
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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[] = {
> 
>

Patch
diff mbox series 

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
+#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[] = {