| Message ID | 20200408070346.24872-2-abhishekkumar8222@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] oidmap: make oidmap_free independent of struct layout | expand |
On Wed, Apr 08, 2020 at 12:33:46PM +0530, Abhishek Kumar wrote: > 87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified > hashmap_iter_next() to return a hashmap_entry pointer instead of void > pointer. > > However, oidmap_iter_next() is unaware of the struct type containing > oidmap_entry and explicitly returns a void pointer. > > Rework oidmap_iter_next() to include struct type and return appropriate > pointer. This allows for compile-time type checks. Yes, I think returning a pointer to an oidmap_entry makes sense. And then we get type safety, and anybody who wants embed an oidmap_entry can use container_of() to get back to their original struct. But... > +/* > + * Returns the next entry, or NULL if there are no more entries. > + * > + * The entry is of @type (e.g. "struct foo") and has a member of type struct > + * oidmap_entry. > + */ > +#define oidmap_iter_next(iter, type) \ > + (type *) hashmap_iter_next(&(iter)->h_iter) This cast is turning a hashmap_entry into whatever type the caller passed in. But it's doing it with a straight cast. We know that hashmap_entry and oidmap_entry pointers are equivalent, but we don't know where the oidmap_entry is with respect to the user's type. I think oidmap_iter_next() should continue to be a function that returns an oidmap_entry pointer (and use container_of_or_null() to get to it from the hashmap_entry, even though we know the offset is 0). And then the caller can either use container_of() to get to their original struct, or we can provide a helper macro. See the difference between hashmap_iter_first() and hashmap_iter_first_entry(). There's no hashmap_iter_next_entry(). There could be, but instead it skipped straight to hashmap_for_each_entry(), which uses an offset within the variable rather than the type. But likewise, we could add oidmap_for_each_entry() here. > diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c > index 0acf99931e..a28bf007a8 100644 > --- a/t/helper/test-oidmap.c > +++ b/t/helper/test-oidmap.c > @@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv) > > struct oidmap_iter iter; > oidmap_iter_init(&map, &iter); > - while ((entry = oidmap_iter_next(&iter))) > + while ((entry = oidmap_iter_next(&iter, struct test_entry))) > printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); This works because "test_entry" has the oidmap_entry at the start. But it wouldn't work with a struct where that wasn't the case, nor would it provide any compile-time safety (because of the cast). Note that if we do want to support that and get type safety (and I think it is worth doing), oidmap_get() would need similar treatment (it returns a void pointer, but it is really a pointer to an oidmap_entry). And I guess oidmap_put() and oidmap_remove(), which returns pointers to existing entries. -Peff
diff --git a/oidmap.h b/oidmap.h index c66a83ab1d..5d6b34a7ce 100644 --- a/oidmap.h +++ b/oidmap.h @@ -76,18 +76,21 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter hashmap_iter_init(&map->map, &iter->h_iter); } -static inline void *oidmap_iter_next(struct oidmap_iter *iter) -{ - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)hashmap_iter_next(&iter->h_iter); -} +/* + * Returns the next entry, or NULL if there are no more entries. + * + * The entry is of @type (e.g. "struct foo") and has a member of type struct + * oidmap_entry. + */ +#define oidmap_iter_next(iter, type) \ + (type *) hashmap_iter_next(&(iter)->h_iter) -static inline void *oidmap_iter_first(struct oidmap *map, - struct oidmap_iter *iter) -{ - oidmap_iter_init(map, iter); - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)oidmap_iter_next(iter); -} +/* + * Returns the first entry in @map using @iter, where the entry is of @type + * (e.g. "struct foo") and has a member of type struct oidmap_entry. + */ +#define oidmap_iter_first(map, iter, type) \ + ({oidmap_iter_init(map, iter); \ + oidmap_iter_next(iter, type); }) #endif diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c index 0acf99931e..a28bf007a8 100644 --- a/t/helper/test-oidmap.c +++ b/t/helper/test-oidmap.c @@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv) struct oidmap_iter iter; oidmap_iter_init(&map, &iter); - while ((entry = oidmap_iter_next(&iter))) + while ((entry = oidmap_iter_next(&iter, struct test_entry))) printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); } else {
87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified hashmap_iter_next() to return a hashmap_entry pointer instead of void pointer. However, oidmap_iter_next() is unaware of the struct type containing oidmap_entry and explicitly returns a void pointer. Rework oidmap_iter_next() to include struct type and return appropriate pointer. This allows for compile-time type checks. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- oidmap.h | 27 +++++++++++++++------------ t/helper/test-oidmap.c | 2 +- 2 files changed, 16 insertions(+), 13 deletions(-)