[ovs-dev] [PATCH v7 09/16] hmap: Use struct for hmap_at_position().
Daniele Di Proietto
diproiettod at vmware.com
Fri Apr 15 23:57:20 UTC 2016
Hi Mark,
On 14/04/2016 05:36, "Kavanagh, Mark B" <mark.b.kavanagh at intel.com> wrote:
>Hi Daniele,
>
>One minor comment inline.
>
>Cheers,
>Mark
>
>>
>>The interface will be more similar to the cmap.
>>
>>Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>>---
>> lib/hmap.c | 26 ++++++++++++--------------
>> lib/hmap.h | 7 ++++++-
>> lib/sset.c | 12 +++++-------
>> lib/sset.h | 7 ++++++-
>> ofproto/ofproto-dpif.c | 8 +++-----
>> 5 files changed, 32 insertions(+), 28 deletions(-)
>>
>>diff --git a/lib/hmap.c b/lib/hmap.c
>>index b70ce51..9462c5e 100644
>>--- a/lib/hmap.c
>>+++ b/lib/hmap.c
>>@@ -236,24 +236,22 @@ hmap_random_node(const struct hmap *hmap)
>> }
>>
>> /* Returns the next node in 'hmap' in hash order, or NULL if no nodes
>>remain in
>>- * 'hmap'. Uses '*bucketp' and '*offsetp' to determine where to begin
>>- * iteration, and stores new values to pass on the next iteration into
>>them
>>- * before returning.
>>+ * 'hmap'. Uses '*pos' to determine where to begin iteration, and
>>updates
>>+ * '*pos' to pass on the next iteration into them before returning.
>> *
>> * It's better to use plain HMAP_FOR_EACH and related functions, since
>>they are
>> * faster and better at dealing with hmaps that change during iteration.
>> *
>>- * Before beginning iteration, store 0 into '*bucketp' and '*offsetp'.
>>- */
>>+ * Before beginning iteration, set '*pos' to all zeros. */
>> struct hmap_node *
>> hmap_at_position(const struct hmap *hmap,
>>- uint32_t *bucketp, uint32_t *offsetp)
>>+ struct hmap_position *pos)
>> {
>> size_t offset;
>> size_t b_idx;
>>
>>- offset = *offsetp;
>>- for (b_idx = *bucketp; b_idx <= hmap->mask; b_idx++) {
>>+ offset = pos->offset;
>>+ for (b_idx = pos->bucket; b_idx <= hmap->mask; b_idx++) {
>> struct hmap_node *node;
>> size_t n_idx;
>>
>>@@ -261,11 +259,11 @@ hmap_at_position(const struct hmap *hmap,
>> n_idx++, node = node->next) {
>> if (n_idx == offset) {
>> if (node->next) {
>>- *bucketp = node->hash & hmap->mask;
>>- *offsetp = offset + 1;
>>+ pos->bucket = node->hash & hmap->mask;
>>+ pos->offset = offset + 1;
>> } else {
>>- *bucketp = (node->hash & hmap->mask) + 1;
>>- *offsetp = 0;
>>+ pos->bucket = (node->hash & hmap->mask) + 1;
>>+ pos->offset = 0;
>> }
>> return node;
>> }
>>@@ -273,8 +271,8 @@ hmap_at_position(const struct hmap *hmap,
>> offset = 0;
>> }
>>
>>- *bucketp = 0;
>>- *offsetp = 0;
>>+ pos->bucket = 0;
>>+ pos->offset = 0;
>> return NULL;
>> }
>>
>>diff --git a/lib/hmap.h b/lib/hmap.h
>>index 08c4719..9a96c5f 100644
>>--- a/lib/hmap.h
>>+++ b/lib/hmap.h
>>@@ -201,8 +201,13 @@ static inline struct hmap_node *hmap_first(const
>>struct hmap *);
>> static inline struct hmap_node *hmap_next(const struct hmap *,
>> const struct hmap_node *);
>>
>>+struct hmap_position {
>>+ unsigned int bucket;
>>+ unsigned int offset;
>>+};
>>+
>> struct hmap_node *hmap_at_position(const struct hmap *,
>>- uint32_t *bucket, uint32_t *offset);
>>+ struct hmap_position *);
>>
>> /* Returns the number of nodes currently in 'hmap'. */
>> static inline size_t
>>diff --git a/lib/sset.c b/lib/sset.c
>>index f9d4fc0..4fd3fae 100644
>>--- a/lib/sset.c
>>+++ b/lib/sset.c
>>@@ -251,21 +251,19 @@ sset_equals(const struct sset *a, const struct
>>sset *b)
>> }
>>
>> /* Returns the next node in 'set' in hash order, or NULL if no nodes
>>remain in
>>- * 'set'. Uses '*bucketp' and '*offsetp' to determine where to begin
>>- * iteration, and stores new values to pass on the next iteration into
>>them
>>- * before returning.
>>+ * 'set'. Uses '*pos' to determine where to begin iteration, and
>>updates
>>+ * '*pos' to pass on the next iteration into them before returning.
>> *
>> * It's better to use plain SSET_FOR_EACH and related functions, since
>>they are
>> * faster and better at dealing with ssets that change during iteration.
>> *
>>- * Before beginning iteration, store 0 into '*bucketp' and '*offsetp'.
>>- */
>>+ * Before beginning iteration, set '*pos' to all zeros. */
>> struct sset_node *
>>-sset_at_position(const struct sset *set, uint32_t *bucketp, uint32_t
>>*offsetp)
>>+sset_at_position(const struct sset *set, struct sset_position *pos)
>> {
>> struct hmap_node *hmap_node;
>>
>>- hmap_node = hmap_at_position(&set->map, bucketp, offsetp);
>>+ hmap_node = hmap_at_position(&set->map, &pos->pos);
>> return SSET_NODE_FROM_HMAP_NODE(hmap_node);
>> }
>>
>>diff --git a/lib/sset.h b/lib/sset.h
>>index 7d1d496..9c2f703 100644
>>--- a/lib/sset.h
>>+++ b/lib/sset.h
>>@@ -64,8 +64,13 @@ char *sset_pop(struct sset *);
>> struct sset_node *sset_find(const struct sset *, const char *);
>> bool sset_contains(const struct sset *, const char *);
>> bool sset_equals(const struct sset *, const struct sset *);
>>+
>>+struct sset_position {
>>+ struct hmap_position pos;
>>+};
>
>[MK] Would a typedef be more appropriate here, as it would avoid the
>additional layer of dereference?
CodingStyle.md says:
"Use typedefs sparingly. Code is clearer if the actual type is
visible at the point of declaration. Do not, in general, declare a
typedef for a struct, union, or enum." and I agree :-), so IMHO
it'd be better to keep it as it is.
What do you think?
Thanks
>>+
>> struct sset_node *sset_at_position(const struct sset *,
>>- uint32_t *bucketp, uint32_t
>>*offsetp);
>>+ struct sset_position *);
>>
>> /* Set operations. */
>> void sset_intersect(struct sset *, const struct sset *);
>>diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>index 530c49a..aceb11f 100644
>>--- a/ofproto/ofproto-dpif.c
>>+++ b/ofproto/ofproto-dpif.c
>>@@ -3558,8 +3558,7 @@ port_get_lacp_stats(const struct ofport *ofport_,
>>struct
>>lacp_slave_stats *stats
>> }
>>
>> struct port_dump_state {
>>- uint32_t bucket;
>>- uint32_t offset;
>>+ struct sset_position pos;
>> bool ghost;
>>
>> struct ofproto_port port;
>>@@ -3587,7 +3586,7 @@ port_dump_next(const struct ofproto *ofproto_,
>>void *state_,
>> state->has_port = false;
>> }
>> sset = state->ghost ? &ofproto->ghost_ports : &ofproto->ports;
>>- while ((node = sset_at_position(sset, &state->bucket,
>>&state->offset))) {
>>+ while ((node = sset_at_position(sset, &state->pos))) {
>> int error;
>>
>> error = port_query_by_name(ofproto_, node->name, &state->port);
>>@@ -3602,8 +3601,7 @@ port_dump_next(const struct ofproto *ofproto_,
>>void *state_,
>>
>> if (!state->ghost) {
>> state->ghost = true;
>>- state->bucket = 0;
>>- state->offset = 0;
>>+ memset(&state->pos, 0, sizeof state->pos);
>> return port_dump_next(ofproto_, state_, port);
>> }
>>
>>--
>>2.1.4
>
More information about the dev
mailing list