| Message ID | 20190106023328.10989-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2019-01-06 04:33:19 +0200, Laurent Pinchart wrote: > 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> > --- > 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..774916f04274 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() > > +#ifdef NDEBUG To match the documentation bellow this should be '#ifndef NDEBUG' right? With this fixed-up, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > +#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..4165cbd654fc 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 is failed > + * > + * 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Sunday, 6 January 2019 13:29:49 EET Niklas Söderlund wrote: > On 2019-01-06 04:33:19 +0200, Laurent Pinchart wrote: > > 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> > > --- > > > > 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..774916f04274 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()> > > +#ifdef NDEBUG > > To match the documentation bellow this should be '#ifndef NDEBUG' right? Absolutely, my bad. I've fixed this. > With this fixed-up, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > +#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..4165cbd654fc 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 is failed > > + * > > + * 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[] = {
Hi Laurent, On Sun, Jan 06, 2019 at 04:33:19AM +0200, Laurent Pinchart wrote: > The ASSERT() macro is similar to the assert() macro defined by the C > standard, but uses the libcamera logging infrastructure. > ../src/libcamera/include/log.h:42:45: error: expected primary-expression before ‘{’ token #define ASSERT(condition) static_cast<void>({ \ ^ ../src/libcamera/media_device.cpp:267:2: note: in expansion of macro ‘ASSERT’ ASSERT(valid_); ^~~~~~ Am I the only one? > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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..774916f04274 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() > > +#ifdef NDEBUG > +#define ASSERT(condition) static_cast<void>({ \ Why do you use a cast to void? Isn't this better wrapped in a canonical "do { } while(0)" ? > + if (condition) \ I feel we should trigger this if the condition is not satisfied, not the other way around, am I wrong? http://www.cplusplus.com/reference/cassert/assert/ > + 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..4165cbd654fc 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 is failed s/is failed/fails ? > + * > + * If \a condition is false, ASSERT() logs an error message with the Fatal log > + * level and aborts program execution. Ah, ok, seems like it should then be "if (!(condition))" according to documentation too. Thanks j > + * > + * 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Monday, 7 January 2019 12:25:13 EET Jacopo Mondi wrote: > On Sun, Jan 06, 2019 at 04:33:19AM +0200, Laurent Pinchart wrote: > > The ASSERT() macro is similar to the assert() macro defined by the C > > standard, but uses the libcamera logging infrastructure. > > ../src/libcamera/include/log.h:42:45: error: expected primary-expression > before ‘{’ token #define ASSERT(condition) static_cast<void>({ \ > ^ > ../src/libcamera/media_device.cpp:267:2: note: in expansion of macro > ‘ASSERT’ ASSERT(valid_); > ^~~~~~ > > Am I the only one? No, after fixing the #if(n)def NDEBUG issue Niklas pointed out, I received the same error. I've fixed it by adding an extra () around the {} statement. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > 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..774916f04274 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()> > > +#ifdef NDEBUG > > +#define ASSERT(condition) static_cast<void>({ \ > > Why do you use a cast to void? Isn't this better wrapped in a > canonical "do { } while(0)" ? I don't know what the pros and cons of each option are, they're both used widely. > > + if (condition) \ > > I feel we should trigger this if the condition is not satisfied, not > the other way around, am I wrong? > http://www.cplusplus.com/reference/cassert/assert/ I must have been really tired when I wrote this... Will fix. > > + 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..4165cbd654fc 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 is failed > > s/is failed/fails ? Fixed. > > + * > > + * If \a condition is false, ASSERT() logs an error message with the > > Fatal log + * level and aborts program execution. > > Ah, ok, seems like it should then be "if (!(condition))" according to > documentation too. At least the documentation is right :-) > > + * > > + * 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[] = {
diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h index 03842be02d0e..774916f04274 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() +#ifdef 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..4165cbd654fc 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 is failed + * + * 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[] = {
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> --- src/libcamera/include/log.h | 9 +++++++++ src/libcamera/log.cpp | 16 ++++++++++++++++ 2 files changed, 25 insertions(+)