[ovs-dev] include/openvswitch/utils.h:

Message ID 1507589977-11186-1-git-send-email-gvrose8192@gmail.com
State Accepted
Headers show
Series
  • [ovs-dev] include/openvswitch/utils.h:
Related show

Commit Message

Gregory Rose Oct. 9, 2017, 10:59 p.m.
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(-)

Comments

Ben Pfaff Oct. 9, 2017, 11:34 p.m. | #1
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
Gregory Rose Oct. 10, 2017, 1:56 a.m. | #2
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.
Ben Pfaff Oct. 10, 2017, 2:59 p.m. | #3
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.

Patch

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 *);