[ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

Eitan Eliahu eliahue at vmware.com
Thu Sep 11 08:57:37 UTC 2014


You have a point. Probably, not all developers would like to have  MSVC pragams inside the code. But, still, I don't like the idea of adding instructions to suppress warnings.
Eitan


-----Original Message-----
From: Alin Serdean [mailto:aserdean at cloudbasesolutions.com] 
Sent: Thursday, September 11, 2014 1:32 AM
To: Eitan Eliahu; Gurucharan Shetty; dev at openvswitch.org
Cc: Gurucharan Shetty
Subject: RE: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

Hi Eitan,
For a limited scope just to suppress the warnings the code will get very cumbersome.
For some variable initialization I do not see what is so inefficient.
Alin.


-----Mesaj original-----
De la: Eitan Eliahu [mailto:eliahue at vmware.com]
Trimis: Thursday, September 11, 2014 1:19 AM
Către: Alin Serdean; Gurucharan Shetty; dev at openvswitch.org
Cc: Gurucharan Shetty
Subiect: RE: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

Hi Alin,
I would prefer to disable the warning in a very limited scope rather than disabling them across all source code. 
Adding instructions just for suppressing warning would be inefficient.
Eitan


-----Original Message-----
From: Alin Serdean [mailto:aserdean at cloudbasesolutions.com]
Sent: Thursday, September 11, 2014 1:07 AM
To: Eitan Eliahu; Gurucharan Shetty; dev at openvswitch.org
Cc: Gurucharan Shetty
Subject: RE: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

Hi Eitan,

I think putting in pragmas to disable warnings is a bit over our hand. 
Either we just disable them in a common include header or we leave them and actually fix the warnings.

Alin.

-----Mesaj original-----
De la: dev [mailto:dev-bounces at openvswitch.org] În numele Eitan Eliahu
Trimis: Wednesday, September 10, 2014 10:21 PM
Către: Gurucharan Shetty; dev at openvswitch.org
Cc: Gurucharan Shetty
Subiect: Re: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

Hi Guru,
You can just suppress the warning by wrapping the code in a pragama warning block:

#pragma warning( push )
#pragma warning( disable : 4707 )
// Some code
#pragma warning( pop ) 

You don't want to initialize to NULL just for suppressing the warning.
Thanks,
Eitan


-----Original Message-----
From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Gurucharan Shetty
Sent: Wednesday, September 10, 2014 12:56 PM
To: dev at openvswitch.org
Cc: Gurucharan Shetty
Subject: [ovs-dev] [PATCH 3/3] Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.

Implementation of OBJECT_OFFSETOF() for non-GNUC compilers like MSVC causes "uninitialized variable" warnings. Since OBJECT_OFFSETOF() is indirectly used through all the *_FOR_EACH() (through ASSIGN_CONTAINER() and  OBJECT_CONTAINING()) macros, the OVS build on Windows gets littered with "uninitialized variable" warnings.
This patch attempts to workaround the problem.

Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
---
 lib/classifier.c |    5 +++--
 lib/classifier.h |    4 ++--
 lib/cmap.h       |    6 +++---
 lib/heap.h       |    2 +-
 lib/hindex.h     |   10 +++++-----
 lib/hmap.h       |   10 +++++-----
 lib/list.h       |   10 +++++-----
 lib/util.h       |    7 +++++++
 8 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/lib/classifier.c b/lib/classifier.c index ae03251..ee737a7 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -1561,7 +1561,7 @@ find_match_wc(const struct cls_subtable *subtable, const struct flow *flow,
               struct flow_wildcards *wc)  {
     uint32_t basis = 0, hash;
-    struct cls_match *rule;
+    struct cls_match *rule = NULL;
     int i;
     struct range ofs;
 
@@ -1767,7 +1767,8 @@ static struct cls_match *  next_rule_in_list__(struct cls_match *rule)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
-    struct cls_match *next = OBJECT_CONTAINING(rule->list.next, next, list);
+    struct cls_match *next = NULL;
+    next = OBJECT_CONTAINING(rule->list.next, next, list);
     return next;
 }
 
diff --git a/lib/classifier.h b/lib/classifier.h index b394724..d6ab144 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -334,7 +334,7 @@ void cls_cursor_advance(struct cls_cursor *);
 #define CLS_FOR_EACH_TARGET(RULE, MEMBER, CLS, TARGET)                  \
     for (struct cls_cursor cursor__ = cls_cursor_start(CLS, TARGET, false); \
          (cursor__.rule                                                 \
-          ? (ASSIGN_CONTAINER(RULE, cursor__.rule, MEMBER),            \
+          ? (INIT_CONTAINER(RULE, cursor__.rule, MEMBER),               \
              true)                                                      \
           : false);                                                     \
          cls_cursor_advance(&cursor__)) @@ -345,7 +345,7 @@ void cls_cursor_advance(struct cls_cursor *);
 #define CLS_FOR_EACH_TARGET_SAFE(RULE, MEMBER, CLS, TARGET)             \
     for (struct cls_cursor cursor__ = cls_cursor_start(CLS, TARGET, true); \
          (cursor__.rule                                                 \
-          ? (ASSIGN_CONTAINER(RULE, cursor__.rule, MEMBER),            \
+          ? (INIT_CONTAINER(RULE, cursor__.rule, MEMBER),               \
              cls_cursor_advance(&cursor__),                             \
              true)                                                      \
           : false);                                                     \
diff --git a/lib/cmap.h b/lib/cmap.h
index 038db6c..793202d 100644
--- a/lib/cmap.h
+++ b/lib/cmap.h
@@ -114,11 +114,11 @@ void cmap_replace(struct cmap *, struct cmap_node *old_node,
  * to change during iteration.  It may be very slightly faster.
  */
 #define CMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, CMAP)       \
-    for (ASSIGN_CONTAINER(NODE, cmap_find(CMAP, HASH), MEMBER); \
+    for (INIT_CONTAINER(NODE, cmap_find(CMAP, HASH), MEMBER);   \
          (NODE) != OBJECT_CONTAINING(NULL, NODE, MEMBER);       \
          ASSIGN_CONTAINER(NODE, cmap_node_next(&(NODE)->MEMBER), MEMBER))
 #define CMAP_FOR_EACH_WITH_HASH_PROTECTED(NODE, MEMBER, HASH, CMAP)        \
-    for (ASSIGN_CONTAINER(NODE, cmap_find_locked(CMAP, HASH), MEMBER);  \
+    for (INIT_CONTAINER(NODE, cmap_find_locked(CMAP, HASH), MEMBER);    \
          (NODE) != OBJECT_CONTAINING(NULL, NODE, MEMBER);               \
          ASSIGN_CONTAINER(NODE, cmap_node_next_protected(&(NODE)->MEMBER), \
                           MEMBER))
@@ -174,7 +174,7 @@ struct cmap_node *cmap_find_protected(const struct cmap *, uint32_t hash);
 
 #define CMAP_CURSOR_FOR_EACH__(NODE, CURSOR, MEMBER)    \
     ((CURSOR)->node                                     \
-     ? (ASSIGN_CONTAINER(NODE, (CURSOR)->node, MEMBER), \
+     ? (INIT_CONTAINER(NODE, (CURSOR)->node, MEMBER),   \
         cmap_cursor_advance(CURSOR),                    \
         true)                                           \
      : false)
diff --git a/lib/heap.h b/lib/heap.h
index 870f582..8de9ea6 100644
--- a/lib/heap.h
+++ b/lib/heap.h
@@ -68,7 +68,7 @@ void heap_rebuild(struct heap *);
  * element. */
 #define HEAP_FOR_EACH(NODE, MEMBER, HEAP)                           \
     for (((HEAP)->n > 0                                             \
-          ? ASSIGN_CONTAINER(NODE, (HEAP)->array[1], MEMBER)        \
+          ? INIT_CONTAINER(NODE, (HEAP)->array[1], MEMBER)          \
           : ((NODE) = NULL, (void) 0));                               \
          (NODE) != NULL;                                            \
          ((NODE)->MEMBER.idx < (HEAP)->n                            \
diff --git a/lib/hindex.h b/lib/hindex.h index 631fd48..416da05 100644
--- a/lib/hindex.h
+++ b/lib/hindex.h
@@ -128,7 +128,7 @@ void hindex_remove(struct hindex *, struct hindex_node *);
  * Evaluates HASH only once.
  */
 #define HINDEX_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HINDEX)               \
-    for (ASSIGN_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), MEMBER); \
+    for (INIT_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), 
+ MEMBER); \
          NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                     \
          ASSIGN_CONTAINER(NODE, (NODE)->MEMBER.s, MEMBER))
 
@@ -149,16 +149,16 @@ hindex_node_with_hash(const struct hindex *hindex, size_t hash)
 
 /* Iterates through every node in HINDEX. */
 #define HINDEX_FOR_EACH(NODE, MEMBER, HINDEX)                           \
-    for (ASSIGN_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);          \
+    for (INIT_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);            \
          NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                 \
          ASSIGN_CONTAINER(NODE, hindex_next(HINDEX, &(NODE)->MEMBER), MEMBER))
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash index but its members remain accessible and intact). */
-#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)                \
-    for (ASSIGN_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);          \
+#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)              \
+    for (INIT_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);          \
          (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)                 \
-          ? ASSIGN_CONTAINER(NEXT, hindex_next(HINDEX, &(NODE)->MEMBER), MEMBER), 1 \
+          ? INIT_CONTAINER(NEXT, hindex_next(HINDEX, &(NODE)->MEMBER), 
+ MEMBER), 1 \
           : 0);                                                         \
          (NODE) = (NEXT))
 
diff --git a/lib/hmap.h b/lib/hmap.h
index 9fb83d5..5fcb7a2 100644
--- a/lib/hmap.h
+++ b/lib/hmap.h
@@ -126,12 +126,12 @@ struct hmap_node *hmap_random_node(const struct hmap *);
  * HASH is only evaluated once.
  */
 #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)               \
-    for (ASSIGN_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \
+    for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), 
+ MEMBER); \
          NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                  \
          ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),   \
                           MEMBER))
 #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)               \
-    for (ASSIGN_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), MEMBER); \
+    for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), 
+ MEMBER); \
          NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                  \
          ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), MEMBER))
 
@@ -148,16 +148,16 @@ bool hmap_contains(const struct hmap *, const struct hmap_node *);
 
 /* Iterates through every node in HMAP. */
 #define HMAP_FOR_EACH(NODE, MEMBER, HMAP)                               \
-    for (ASSIGN_CONTAINER(NODE, hmap_first(HMAP), MEMBER);              \
+    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER);                \
          NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER);                  \
          ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash map but its members remain accessible and intact). */
 #define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP)                    \
-    for (ASSIGN_CONTAINER(NODE, hmap_first(HMAP), MEMBER);              \
+    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER);                \
          (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)                  \
-          ? ASSIGN_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), 1 \
+          ? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), 
+ MEMBER), 1 \
           : 0);                                                         \
          (NODE) = (NEXT))
 
diff --git a/lib/list.h b/lib/list.h
index 0da082e..ef6a9db 100644
--- a/lib/list.h
+++ b/lib/list.h
@@ -58,15 +58,15 @@ bool list_is_singleton(const struct list *);  bool list_is_short(const struct list *);
 
 #define LIST_FOR_EACH(ITER, MEMBER, LIST)                               \
-    for (ASSIGN_CONTAINER(ITER, (LIST)->next, MEMBER);                  \
+    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);                    \
          &(ITER)->MEMBER != (LIST);                                     \
          ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
 #define LIST_FOR_EACH_CONTINUE(ITER, MEMBER, LIST)                      \
-    for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER);           \
+    for (INIT_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER);             \
          &(ITER)->MEMBER != (LIST);                                     \
          ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
 #define LIST_FOR_EACH_REVERSE(ITER, MEMBER, LIST)                       \
-    for (ASSIGN_CONTAINER(ITER, (LIST)->prev, MEMBER);                  \
+    for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);                    \
          &(ITER)->MEMBER != (LIST);                                     \
          ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
 #define LIST_FOR_EACH_REVERSE_CONTINUE(ITER, MEMBER, LIST)              \
@@ -74,9 +74,9 @@ bool list_is_short(const struct list *);
          &(ITER)->MEMBER != (LIST);                                     \
          ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
 #define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST)               \
-    for (ASSIGN_CONTAINER(ITER, (LIST)->next, MEMBER);             \
+    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);               \
          (&(ITER)->MEMBER != (LIST)                                \
-          ? ASSIGN_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1 \
+          ? INIT_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1   \
           : 0);                                                    \
          (ITER) = (NEXT))
 
diff --git a/lib/util.h b/lib/util.h
index 261b4b3..a2c6ee9 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -228,6 +228,13 @@ ovs_prefetch_range(const void *start, size_t size)  #define ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER) \
     ((OBJECT) = OBJECT_CONTAINING(POINTER, OBJECT, MEMBER), (void) 0)
 
+/* As explained in the comment above OBJECT_OFFSETOF(), non-GNUC 
+compilers
+ * like MSVC will complain about un-initialized variables if OBJECT
+ * hasn't already been initialized. To prevent such warnings,
+INIT_CONTAINER()
+ * can be used as a wrapper around ASSIGN_CONTAINER. */ #define 
+INIT_CONTAINER(OBJECT, POINTER, MEMBER) \
+    ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER))
+
 /* Given ATTR, and TYPE, cast the ATTR to TYPE by first casting ATTR to
  * (void *). This is to suppress the alignment warning issued by clang. */  #define ALIGNED_CAST(TYPE, ATTR) ((TYPE) (void *) (ATTR))
--
1.7.9.5

_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=vhaaiwGg2o4fdzo1Y4GXNiuZqtaokSkxaJCi51lfBLo%3D%0A&s=916b857d57af38bd52b9bf30cb0dda65b6f0ae970620dcd2800b619fe86a4dee
_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=uzgOHkSF0d95NbtVAcQIQXP0T4OJNQaJOAX%2FZCXrnbk%3D%0A&s=49cccc3419f982168d7a4c74f94787a148fdf7503c6362026faa8c02ae366aa5



More information about the dev mailing list