| Message ID | 1507589977-11186-1-git-send-email-gvrose8192@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] include/openvswitch/utils.h: | expand |
On Mon, Oct 09, 2017 at 03:59:37PM -0700, Greg Rose wrote: > Fix macro for case where build is done with NDEBUG > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > --- > include/openvswitch/util.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > index abebf96..72299e7 100644 > --- a/include/openvswitch/util.h > +++ b/include/openvswitch/util.h > @@ -52,7 +52,7 @@ const char *ovs_get_program_version(void); > ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION); \ > } > #else > -#define ovs_assert(CONDITION) ((void) (CONDITION)) > +#define ovs_assert(CONDITION) ((void) (CONDITION)); > #endif We normally invoke ovs_assert() like a function call, followed by a semicolon. If there's a missing ; somewhere in a user of ovs_assert(), we should fix that. The other case does look weird to me, because in normal usage it will expand to something like: if (...) { ... }; so it might be a good idea to make it a single expression, maybe like this: diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h index abebf96fa3ac..84d7e07ff370 100644 --- a/include/openvswitch/util.h +++ b/include/openvswitch/util.h @@ -48,9 +48,9 @@ const char *ovs_get_program_version(void); * log. */ #ifndef NDEBUG #define ovs_assert(CONDITION) \ - if (!OVS_LIKELY(CONDITION)) { \ - ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION); \ - } + (OVS_LIKELY(CONDITION) \ + ? (void) 0 \ + : ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION)) #else #define ovs_assert(CONDITION) ((void) (CONDITION)) #endif
On 10/09/2017 04:34 PM, Ben Pfaff wrote: > On Mon, Oct 09, 2017 at 03:59:37PM -0700, Greg Rose wrote: > > Fix macro for case where build is done with NDEBUG > > > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > > --- > > include/openvswitch/util.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > > index abebf96..72299e7 100644 > > --- a/include/openvswitch/util.h > > +++ b/include/openvswitch/util.h > > @@ -52,7 +52,7 @@ const char *ovs_get_program_version(void); > > ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION); \ > > } > > #else > > -#define ovs_assert(CONDITION) ((void) (CONDITION)) > > +#define ovs_assert(CONDITION) ((void) (CONDITION)); > > #endif > > We normally invoke ovs_assert() like a function call, followed by a > semicolon. If there's a missing ; somewhere in a user of ovs_assert(), > we should fix that. > > The other case does look weird to me, because in normal usage it will > expand to something like: > if (...) { > ... > }; > so it might be a good idea to make it a single expression, maybe like > this: > > diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > index abebf96fa3ac..84d7e07ff370 100644 > --- a/include/openvswitch/util.h > +++ b/include/openvswitch/util.h > @@ -48,9 +48,9 @@ const char *ovs_get_program_version(void); > * log. */ > #ifndef NDEBUG > #define ovs_assert(CONDITION) \ > - if (!OVS_LIKELY(CONDITION)) { \ > - ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION); \ > - } > + (OVS_LIKELY(CONDITION) \ > + ? (void) 0 \ > + : ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION)) > #else > #define ovs_assert(CONDITION) ((void) (CONDITION)) > #endif > Yep, that makes more sense to me... I was just trying to do something else and ran into this so didn't think on it much. Looks good to me! Thanks Ben.
On Mon, Oct 09, 2017 at 06:56:21PM -0700, Greg Rose wrote: > On 10/09/2017 04:34 PM, Ben Pfaff wrote: > >On Mon, Oct 09, 2017 at 03:59:37PM -0700, Greg Rose wrote: > >> Fix macro for case where build is done with NDEBUG > >> > >> Signed-off-by: Greg Rose <gvrose8192@gmail.com> > >> --- > >> include/openvswitch/util.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > >> index abebf96..72299e7 100644 > >> --- a/include/openvswitch/util.h > >> +++ b/include/openvswitch/util.h > >> @@ -52,7 +52,7 @@ const char *ovs_get_program_version(void); > >> ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION); \ > >> } > >> #else > >> -#define ovs_assert(CONDITION) ((void) (CONDITION)) > >> +#define ovs_assert(CONDITION) ((void) (CONDITION)); > >> #endif > > > >We normally invoke ovs_assert() like a function call, followed by a > >semicolon. If there's a missing ; somewhere in a user of ovs_assert(), > >we should fix that. > > > >The other case does look weird to me, because in normal usage it will > >expand to something like: > > if (...) { > > ... > > }; > >so it might be a good idea to make it a single expression, maybe like > >this: > > > >diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > >index abebf96fa3ac..84d7e07ff370 100644 > >--- a/include/openvswitch/util.h > >+++ b/include/openvswitch/util.h > >@@ -48,9 +48,9 @@ const char *ovs_get_program_version(void); > > * log. */ > > #ifndef NDEBUG > > #define ovs_assert(CONDITION) \ > >- if (!OVS_LIKELY(CONDITION)) { \ > >- ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION); \ > >- } > >+ (OVS_LIKELY(CONDITION) \ > >+ ? (void) 0 \ > >+ : ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION)) > > #else > > #define ovs_assert(CONDITION) ((void) (CONDITION)) > > #endif > > > Yep, that makes more sense to me... I was just trying to do something else and ran into this > so didn't think on it much. > > Looks good to me! Thanks! I applied this to master.
diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h index abebf96..72299e7 100644 --- a/include/openvswitch/util.h +++ b/include/openvswitch/util.h @@ -52,7 +52,7 @@ const char *ovs_get_program_version(void); ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION); \ } #else -#define ovs_assert(CONDITION) ((void) (CONDITION)) +#define ovs_assert(CONDITION) ((void) (CONDITION)); #endif OVS_NO_RETURN void ovs_assert_failure(const char *, const char *, const char *);
Fix macro for case where build is done with NDEBUG Signed-off-by: Greg Rose <gvrose8192@gmail.com> --- include/openvswitch/util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)