Hi Gaëtan,
On 08/31/2018 06:53 PM, Gaëtan Rivet wrote:
Hi Qiaobin,
This work seems interesting, but is difficult to follow because
the previous discussion is not referenced.
You can find a how-to there:
http://doc.dpdk.org/guides/contributing/patches.html#sending-patches
--in-reply-to is useful to check which comments were already made and
understand the work previously done on a patchset.
Thanks for bringing this to our attention.
+/* istate stands for internal state. */
Is a name requiring a comment to explain a good name?
Maybe rte_hash_iterator_priv?
+struct rte_hash_iterator_istate {
+ const struct rte_hash *h;
+ uint32_t next;
+ uint32_t total_entries;
+};
We agree that the suffix _priv is better.
You should check that your private structure does not grow beyond
the public one, using RTE_BUILD_BUG_ON(sizeof(priv) < sizeof(pub)) somewhere.
We have overlooked the macro RTE_BUILD_BUG_ON(). We'll use it.
"rte_hash_iterator_[i]state" seems unnecessarily verbose.
The memory you are manipulating through this variable is already holding
the state of your iterator. It is useless to append "_state".
struct rte_hash_iterator_priv *state;
is also clear and reads better.
On the other hand "h" is maybe not verbose enough. Why not "hash"?
We'll keep the parameter name "state" and rename the variable
"__state" to "it" as you suggest in a comment later in your email.
About the variable "h", we are following the coding convention in
the library. You can find plenty of examples of using "h" for a hash table.
Also, please do not align field names in a structure. It forces
future changes to either break the pattern or edit the whole structure
when someone attempts to insert a field with a name that is too long.
Your suggestion goes against the coding style of DPDK. See section
"1.5.5. Structure Declarations" on the page:
https://doc.dpdk.org/guides-18.08/contributing/coding_style.html
+
+int32_t
+rte_hash_iterator_init(const struct rte_hash *h,
+ struct rte_hash_iterator_state *state)
+{
+ struct rte_hash_iterator_istate *__state;
Please do not use the "__" prefix to convey that
you are using a private version of the structure.
You could use "istate" or "it", the common shorthand for
iterator handles.
We'll do it as explained before.
int32_t
-rte_hash_iterate(const struct rte_hash *h, const void **key, void **data,
uint32_t *next)
+rte_hash_iterate(
+ struct rte_hash_iterator_state *state, const void **key, void **data)
Why an empty first line of parameters here?
rte_hash_iterate(struct rte_hash_iterator_state *state,
const void **key,
void **data)
reads better.
Okay.
+
+/* istate stands for internal state. */
+struct rte_hash_iterator_conflict_entries_istate {
I find "conflict_entries" awkward, how about
rte_hash_dup_iterator
instead? It is shorter and conveys that you will iterate duplicate
entries.
Yipeng Wang suggested the expression "conflict_entries" in his
review of the first version of this patch. You find his suggestion here:
http://mails.dpdk.org/archives/dev/2018-August/109103.html
I find the name "dup" misleading because it suggests that the
returned entries have the same key or refer to the same object. For
example, the file descriptor returned by dup(2) refers to the same file.
+ const struct rte_hash *h;
+ uint32_t vnext;
+ uint32_t primary_bidx;
+ uint32_t secondary_bidx;
+};
+
+int32_t __rte_experimental
+rte_hash_iterator_conflict_entries_init_with_hash(const struct rte_hash *h,
rte_hash_dup_iterator_init() maybe?
Why is _with_hash mentioned here? Is it possible to initialize this kind
of iterator without a reference to compare against? That this reference
is an rte_hash is already given by the parameter list.
In any case, 49 characters for a name is too long.
Honnappa Nagarahalli suggested adding the suffix "_with_hash" during
his review of the second version of this patch. His argument went as
follows: "Let me elaborate. For the API 'rte_hash_lookup', there are
multiple variations such as 'rte_hash_lookup_with_hash',
'rte_hash_lookup_data', 'rte_hash_lookup_with_hash_data' etc. We do not
need to create similar variations for
'rte_hash_iterate_conflict_entries' API right now. But the naming of the
API should be such that these variations can be created in the future."
You find a copy of his original message here:
https://www.mail-archive.com/dev@dpdk.org/msg109653.html
+int32_t __rte_experimental
+rte_hash_iterate_conflict_entries(
+ struct rte_hash_iterator_state *state, const void **key, void **data)
How about "rte_hash_dup_next()"?
Also, please break the parameter list instead of having an empty first
line.
We are preserving the name convention used in DPDK (e.g.
rte_hash_iterate()).
We'll break the parameter list to avoid the empty first line.
diff --git a/lib/librte_hash/rte_hash_version.map
b/lib/librte_hash/rte_hash_version.map
index e216ac8e2..301d4638c 100644
--- a/lib/librte_hash/rte_hash_version.map
+++ b/lib/librte_hash/rte_hash_version.map
@@ -24,6 +24,7 @@ DPDK_2.1 {
rte_hash_add_key_data;
rte_hash_add_key_with_hash_data;
+ rte_hash_iterator_init;
rte_hash_iterate;
rte_hash_lookup_bulk_data;
rte_hash_lookup_data;
@@ -53,3 +54,10 @@ DPDK_18.08 {
rte_hash_count;
} DPDK_16.07;
+
+EXPERIMENTAL {
+ global:
+
+ rte_hash_iterator_conflict_entries_init_with_hash;
+ rte_hash_iterate_conflict_entries;
+};
A blank line may be missing here.
Do you mean a blank line before "};"?
Thank you for the careful review.
[ ]'s
Michel Machado
