* [PATCH 1/2] oidmap: make oidmap_free independent of struct layout
@ 2020-04-08 4:06 Abhishek Kumar
2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Abhishek Kumar @ 2020-04-08 4:06 UTC (permalink / raw)
To: git
c8e424c introduced hashmap_free_entries, which can free any struct
pointer, regardless of the hashmap_entry field offset.
oidmap does not make use of this flexibilty, hardcoding the offset to
zero instead. Let's fix this by passing struct type and member to
hashmap_free_entries.
Additionally, removes an erroneous semi-colon at the end of
hashmap_free_entries macro.
Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
hashmap.h | 2 +-
oidmap.c | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/hashmap.h b/hashmap.h
index 79ae9f80de..6d0a65a39f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
* where @member is the hashmap_entry struct used to associate with @map
*/
#define hashmap_free_entries(map, type, member) \
- hashmap_free_(map, offsetof(type, member));
+ hashmap_free_(map, offsetof(type, member))
/* hashmap_entry functions */
diff --git a/oidmap.c b/oidmap.c
index 423aa014a3..65d63787a8 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)
if (!map)
return;
- /* TODO: make oidmap itself not depend on struct layouts */
- hashmap_free_(&map->map, free_entries ? 0 : -1);
+ if (free_entries)
+ hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);
+ else
+ hashmap_free(&map->map);
}
void *oidmap_get(const struct oidmap *map, const struct object_id *key)
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] oidmap: rework iterators to return typed pointer
2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar
@ 2020-04-08 4:06 ` Abhishek Kumar
2020-04-08 6:02 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Junio C Hamano
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Abhishek Kumar @ 2020-04-08 4:06 UTC (permalink / raw)
To: git
87571c3 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 explicilty returns a void pointer.
Rework oidmap_iter_* 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(-)
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 {
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout
2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar
2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar
@ 2020-04-08 6:02 ` Junio C Hamano
2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2020-04-08 6:02 UTC (permalink / raw)
To: Abhishek Kumar; +Cc: git
Abhishek Kumar <abhishekkumar8222@gmail.com> writes:
> c8e424c introduced hashmap_free_entries, which can free any struct
> pointer, regardless of the hashmap_entry field offset.
c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06)
introduced hashmap_free_entries(), which ...
is how we refer to existing commits and functions.
> oidmap does not make use of this flexibilty, hardcoding the offset to
> zero instead. Let's fix this by passing struct type and member to
> hashmap_free_entries.
Makes sense.
> Additionally, removes an erroneous semi-colon at the end of
> hashmap_free_entries macro.
s/removes/remove/
Good eyes. Of course, your if/else would have broken without this
fix ;-)
Looking good. Thanks.
> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
> hashmap.h | 2 +-
> oidmap.c | 6 ++++--
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hashmap.h b/hashmap.h
> index 79ae9f80de..6d0a65a39f 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
> * where @member is the hashmap_entry struct used to associate with @map
> */
> #define hashmap_free_entries(map, type, member) \
> - hashmap_free_(map, offsetof(type, member));
> + hashmap_free_(map, offsetof(type, member))
>
> /* hashmap_entry functions */
>
> diff --git a/oidmap.c b/oidmap.c
> index 423aa014a3..65d63787a8 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)
> if (!map)
> return;
>
> - /* TODO: make oidmap itself not depend on struct layouts */
> - hashmap_free_(&map->map, free_entries ? 0 : -1);
> + if (free_entries)
> + hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);
> + else
> + hashmap_free(&map->map);
> }
>
> void *oidmap_get(const struct oidmap *map, const struct object_id *key)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] oidmap: make oidmap_free independent of struct layout
2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar
2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar
2020-04-08 6:02 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Junio C Hamano
@ 2020-04-08 7:03 ` Abhishek Kumar
2020-04-08 7:03 ` [PATCH v2 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar
2020-04-08 21:54 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Jeff King
2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar
4 siblings, 1 reply; 10+ messages in thread
From: Abhishek Kumar @ 2020-04-08 7:03 UTC (permalink / raw)
To: git
c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06)
introduced hashmap_free_entries(), which can free any struct pointer,
regardless of the hashmap_entry field offset.
oidmap does not make use of this flexibilty, hardcoding the offset to
zero instead. Let's fix this by passing struct type and member to
hashmap_free_entries().
Additionally, remove an erroneous semi-colon at the end of
hashmap_free_entries() macro.
Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
Changes in v2:
- Fix references to earlier commits.
- Grammar fixes.
hashmap.h | 2 +-
oidmap.c | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/hashmap.h b/hashmap.h
index 79ae9f80de..6d0a65a39f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
* where @member is the hashmap_entry struct used to associate with @map
*/
#define hashmap_free_entries(map, type, member) \
- hashmap_free_(map, offsetof(type, member));
+ hashmap_free_(map, offsetof(type, member))
/* hashmap_entry functions */
diff --git a/oidmap.c b/oidmap.c
index 423aa014a3..65d63787a8 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)
if (!map)
return;
- /* TODO: make oidmap itself not depend on struct layouts */
- hashmap_free_(&map->map, free_entries ? 0 : -1);
+ if (free_entries)
+ hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);
+ else
+ hashmap_free(&map->map);
}
void *oidmap_get(const struct oidmap *map, const struct object_id *key)
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] oidmap: rework iterators to return typed pointer
2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar
@ 2020-04-08 7:03 ` Abhishek Kumar
2020-04-08 22:08 ` Jeff King
0 siblings, 1 reply; 10+ messages in thread
From: Abhishek Kumar @ 2020-04-08 7:03 UTC (permalink / raw)
To: git
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(-)
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 {
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout
2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar
` (2 preceding siblings ...)
2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar
@ 2020-04-08 21:54 ` Jeff King
2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar
4 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2020-04-08 21:54 UTC (permalink / raw)
To: Abhishek Kumar; +Cc: git
On Wed, Apr 08, 2020 at 09:36:58AM +0530, Abhishek Kumar wrote:
> c8e424c introduced hashmap_free_entries, which can free any struct
> pointer, regardless of the hashmap_entry field offset.
>
> oidmap does not make use of this flexibilty, hardcoding the offset to
> zero instead. Let's fix this by passing struct type and member to
> hashmap_free_entries.
I'm not sure how much this improves anything.
We're now telling the hashmap code how to get to our "struct
oidmap_entry" pointer by using the correct offset there. But we know
that offset will always be 0, because we put our internal hashmap entry
at the start of oidmap_entry.
What you can't do is:
struct foo {
int first_member;
struct oidmap_entry not_the_first_member;
};
and work directly with "struct foo" pointers. You have to convert
oidmap_entry pointers into foo pointers with container_of() or similar.
And that is true both before and after your patch.
That said, I don't mind doing this as a cleanup; there's a subtle
dependency on the location of internal_entry within object_entry,
and this would move towards getting rid of it.
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] oidmap: rework iterators to return typed pointer
2020-04-08 7:03 ` [PATCH v2 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar
@ 2020-04-08 22:08 ` Jeff King
0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2020-04-08 22:08 UTC (permalink / raw)
To: Abhishek Kumar; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/3] oidmap: make oidmap_free independent of struct layout
2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar
` (3 preceding siblings ...)
2020-04-08 21:54 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Jeff King
@ 2020-04-25 2:59 ` Abhishek Kumar
2020-04-25 2:59 ` [PATCH v3 2/3] oidmap: rework iterators to return typed pointer Abhishek Kumar
2020-04-25 2:59 ` [PATCH v3 3/3] oidmap: return oidmap_entry pointers Abhishek Kumar
4 siblings, 2 replies; 10+ messages in thread
From: Abhishek Kumar @ 2020-04-25 2:59 UTC (permalink / raw)
To: git
c8e424c introduced hashmap_free_entries, which can free any struct
pointer, regardless of the hashmap_entry field offset.
oidmap does not make use of this flexibilty, hardcoding the offset to
zero instead. Let's fix this by passing struct type and member to
hashmap_free_entries.
Additionally, remove an erroneous semi-colon at the end of
hashmap_free_entries macro.
Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
hashmap.h | 2 +-
oidmap.c | 7 +++++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/hashmap.h b/hashmap.h
index 79ae9f80de..6d0a65a39f 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);
* where @member is the hashmap_entry struct used to associate with @map
*/
#define hashmap_free_entries(map, type, member) \
- hashmap_free_(map, offsetof(type, member));
+ hashmap_free_(map, offsetof(type, member))
/* hashmap_entry functions */
diff --git a/oidmap.c b/oidmap.c
index 423aa014a3..44345a8cf2 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -26,8 +26,11 @@ void oidmap_free(struct oidmap *map, int free_entries)
if (!map)
return;
- /* TODO: make oidmap itself not depend on struct layouts */
- hashmap_free_(&map->map, free_entries ? 0 : -1);
+ if (free_entries)
+ hashmap_free_entries(
+ &map->map, struct oidmap_entry, internal_entry);
+ else
+ hashmap_free(&map->map);
}
void *oidmap_get(const struct oidmap *map, const struct object_id *key)
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] oidmap: rework iterators to return typed pointer
2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar
@ 2020-04-25 2:59 ` Abhishek Kumar
2020-04-25 2:59 ` [PATCH v3 3/3] oidmap: return oidmap_entry pointers Abhishek Kumar
1 sibling, 0 replies; 10+ messages in thread
From: Abhishek Kumar @ 2020-04-25 2:59 UTC (permalink / raw)
To: git
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.
Rewrite oidmap_iter_next() to return oidmap_entry pointer and add
helper macros to return pointers to container struct.
Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
oidmap.h | 41 +++++++++++++++++++++++++++++++++++------
t/helper/test-oidmap.c | 7 ++++---
2 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/oidmap.h b/oidmap.h
index c66a83ab1d..d79b087ab0 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -66,6 +66,8 @@ void *oidmap_put(struct oidmap *map, void *entry);
*/
void *oidmap_remove(struct oidmap *map, const struct object_id *key);
+#define oidmap_entry_from_hashmap_entry(entry) \
+ container_of_or_null(entry, struct oidmap_entry, internal_entry)
struct oidmap_iter {
struct hashmap_iter h_iter;
@@ -76,18 +78,45 @@ 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)
+/* Returns the next oidmap_entry, or NULL if there are no more entries. */
+static inline struct oidmap_entry *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);
+ return oidmap_entry_from_hashmap_entry(
+ hashmap_iter_next(&iter->h_iter));
}
-static inline void *oidmap_iter_first(struct oidmap *map,
+/* Initializes the iterator and returns the first entry, if any. */
+static inline struct oidmap_entry *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);
+ return oidmap_iter_next(iter);
}
+/*
+ * Returns the first entry in @map using @iter, where the entry if of
+ * @type (e.g. "struct foo") and @member is the name of the
+ * "struct oidmap_entry" in @type
+ */
+#define oidmap_iter_first_entry(map, iter, type, member) \
+ container_of_or_null(oidmap_iter_first(map, iter), type, member)
+
+/* Internal macro for oidmap_for_each_entry */
+#define oidmap_iter_next_entry_offset(iter, offset) \
+ container_of_or_null_offset(oidmap_iter_next(iter), offset)
+
+/* Internal macro for oidmap_for_each_entry */
+#define oidmap_iter_first_entry_offset(map, iter, offset) \
+ container_of_or_null_offset(oidmap_iter_first(map, iter), offset)
+
+/*
+ * Iterate through @map using @iter, @var is a pointer to a type
+ * containing a @member which is a "struct oidmap_entry"
+ */
+#define oidmap_for_each_entry(map, iter, var, member) \
+ for (var = oidmap_iter_first_entry_offset(map, iter, \
+ OFFSETOF_VAR(var, member)); \
+ var; \
+ var = oidmap_iter_next_entry_offset(iter, \
+ OFFSETOF_VAR(var, member)))
#endif
diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
index 0acf99931e..3f599b21b8 100644
--- a/t/helper/test-oidmap.c
+++ b/t/helper/test-oidmap.c
@@ -95,9 +95,10 @@ int cmd__oidmap(int argc, const char **argv)
} else if (!strcmp("iterate", cmd)) {
struct oidmap_iter iter;
- oidmap_iter_init(&map, &iter);
- while ((entry = oidmap_iter_next(&iter)))
- printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name);
+ struct test_entry *ent;
+
+ oidmap_for_each_entry(&map, &iter, ent, entry)
+ printf("%s %s\n", oid_to_hex(&ent->entry.oid), ent->name);
} else {
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] oidmap: return oidmap_entry pointers
2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar
2020-04-25 2:59 ` [PATCH v3 2/3] oidmap: rework iterators to return typed pointer Abhishek Kumar
@ 2020-04-25 2:59 ` Abhishek Kumar
1 sibling, 0 replies; 10+ messages in thread
From: Abhishek Kumar @ 2020-04-25 2:59 UTC (permalink / raw)
To: git
oidmap_entry is usually embedded as first member of user data
structures. Since oidmap is unaware of structure, it returns an void
pointer for oidmap_get(), oidmap_put() and oidmap_remove().
This pointer is rather a pointer to a oidmap entry. Teach the functions
to return a oidmap_entry pointer and add helper macros to return
pointers to container struct.
Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
---
list-objects-filter.c | 8 +++++---
oidmap.c | 21 ++++++++++++---------
oidmap.h | 42 ++++++++++++++++++++++++++++++++++++------
replace-object.c | 7 ++++---
sequencer.c | 28 ++++++++++++++++++----------
t/helper/test-oidmap.c | 8 +++++---
6 files changed, 80 insertions(+), 34 deletions(-)
diff --git a/list-objects-filter.c b/list-objects-filter.c
index 1e8d4e763d..16b45d913e 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -183,13 +183,15 @@ static enum list_objects_filter_result filter_trees_depth(
return include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;
case LOFS_BEGIN_TREE:
- seen_info = oidmap_get(
- &filter_data->seen_at_depth, &obj->oid);
+ oidmap_get_entry(seen_info, base, &filter_data->seen_at_depth,
+ &obj->oid);
+
if (!seen_info) {
seen_info = xcalloc(1, sizeof(*seen_info));
oidcpy(&seen_info->base.oid, &obj->oid);
seen_info->depth = filter_data->current_depth;
- oidmap_put(&filter_data->seen_at_depth, seen_info);
+ oidmap_put_entry(seen_info, base,
+ &filter_data->seen_at_depth);
already_seen = 0;
} else {
already_seen =
diff --git a/oidmap.c b/oidmap.c
index 44345a8cf2..0e64c0e8a2 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -33,15 +33,18 @@ void oidmap_free(struct oidmap *map, int free_entries)
hashmap_free(&map->map);
}
-void *oidmap_get(const struct oidmap *map, const struct object_id *key)
+struct oidmap_entry *oidmap_get(const struct oidmap *map,
+ const struct object_id *key)
{
if (!map->map.cmpfn)
return NULL;
- return hashmap_get_from_hash(&map->map, oidhash(key), key);
+ return oidmap_entry_from_hashmap_entry(
+ hashmap_get_from_hash(&map->map, oidhash(key), key));
}
-void *oidmap_remove(struct oidmap *map, const struct object_id *key)
+struct oidmap_entry *oidmap_remove(struct oidmap *map,
+ const struct object_id *key)
{
struct hashmap_entry entry;
@@ -49,16 +52,16 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)
oidmap_init(map, 0);
hashmap_entry_init(&entry, oidhash(key));
- return hashmap_remove(&map->map, &entry, key);
+ return oidmap_entry_from_hashmap_entry(
+ hashmap_remove(&map->map, &entry, key));
}
-void *oidmap_put(struct oidmap *map, void *entry)
+struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry)
{
- struct oidmap_entry *to_put = entry;
-
if (!map->map.cmpfn)
oidmap_init(map, 0);
- hashmap_entry_init(&to_put->internal_entry, oidhash(&to_put->oid));
- return hashmap_put(&map->map, &to_put->internal_entry);
+ hashmap_entry_init(&entry->internal_entry, oidhash(&entry->oid));
+ return oidmap_entry_from_hashmap_entry(
+ hashmap_put(&map->map, &entry->internal_entry));
}
diff --git a/oidmap.h b/oidmap.h
index d79b087ab0..f250f0bf87 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -46,25 +46,55 @@ void oidmap_free(struct oidmap *map, int free_entries);
/*
* Returns the oidmap entry for the specified oid, or NULL if not found.
*/
-void *oidmap_get(const struct oidmap *map,
+struct oidmap_entry *oidmap_get(const struct oidmap *map,
const struct object_id *key);
+/*
+ * Returns pointer to container struct of the oidmap entry for the
+ * specified oid, or NULL if not found.
+ */
+#define oidmap_get_entry(var, member, map, key) \
+ var = container_of_or_null_offset( \
+ oidmap_get(map, key), \
+ OFFSETOF_VAR(var, member))
/*
* Adds or replaces an oidmap entry.
*
- * ((struct oidmap_entry *) entry)->internal_entry will be populated by this
- * function.
- *
* Returns the replaced entry, or NULL if not found (i.e. the entry was added).
*/
-void *oidmap_put(struct oidmap *map, void *entry);
+struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry);
+
+/*
+ * Adds or replaces an oidmap entry.
+ *
+ * Returns pointer to container struct of replaced entry, or NULL if
+ * not found (i.e. the entry was added).
+ *
+ * The struct type of @var contains a "struct oidmap_entry" @member.
+ */
+#define oidmap_put_entry(var, member, map) \
+ container_of_or_null_offset( \
+ oidmap_put(map, &var->member), \
+ OFFSETOF_VAR(var, member))
/*
* Removes an oidmap entry matching the specified oid.
*
* Returns the removed entry, or NULL if not found.
*/
-void *oidmap_remove(struct oidmap *map, const struct object_id *key);
+struct oidmap_entry *oidmap_remove(struct oidmap *map,
+ const struct object_id *key);
+
+/*
+ * Removes an oidmap entry matching the specified oid.
+ *
+ * Returns pointer to container struct of the removed entry, or NULL if
+ * not found.
+ */
+#define oidmap_remove_entry(var, member, map, key) \
+ var = container_of_or_null_offset( \
+ oidmap_remove(map, key), \
+ OFFSETOF_VAR(var, member))
#define oidmap_entry_from_hashmap_entry(entry) \
container_of_or_null(entry, struct oidmap_entry, internal_entry)
diff --git a/replace-object.c b/replace-object.c
index 7bd9aba6ee..ab0354db7a 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -26,7 +26,7 @@ static int register_replace_ref(struct repository *r,
oidcpy(&repl_obj->replacement, oid);
/* Register new object */
- if (oidmap_put(r->objects->replace_map, repl_obj))
+ if (oidmap_put_entry(repl_obj, original, r->objects->replace_map))
die(_("duplicate replace ref: %s"), refname);
return 0;
@@ -73,8 +73,9 @@ const struct object_id *do_lookup_replace_object(struct repository *r,
/* Try to recursively replace the object */
while (depth-- > 0) {
- struct replace_object *repl_obj =
- oidmap_get(r->objects->replace_map, cur);
+ struct replace_object *repl_obj;
+ oidmap_get_entry(repl_obj, original, r->objects->replace_map,
+ cur);
if (!repl_obj)
return cur;
cur = &repl_obj->replacement;
diff --git a/sequencer.c b/sequencer.c
index f30bb73c70..012e7865db 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4506,7 +4506,8 @@ static const char *label_oid(struct object_id *oid, const char *label,
struct object_id dummy;
int i;
- string_entry = oidmap_get(&state->commit2label, oid);
+ oidmap_get_entry(string_entry, entry, &state->commit2label, oid);
+
if (string_entry)
return string_entry->string;
@@ -4606,7 +4607,7 @@ static const char *label_oid(struct object_id *oid, const char *label,
FLEX_ALLOC_STR(string_entry, string, label);
oidcpy(&string_entry->entry.oid, oid);
- oidmap_put(&state->commit2label, string_entry);
+ oidmap_put_entry(string_entry, entry, &state->commit2label);
return string_entry->string;
}
@@ -4645,7 +4646,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
struct object_id *oid = &revs->cmdline.rev[0].item->oid;
FLEX_ALLOC_STR(entry, string, "onto");
oidcpy(&entry->entry.oid, oid);
- oidmap_put(&state.commit2label, entry);
+ oidmap_put_entry(entry, entry, /* member name */
+ &state.commit2label);
FLEX_ALLOC_STR(onto_label_entry, label, "onto");
hashmap_entry_init(&onto_label_entry->entry, strihash("onto"));
@@ -4689,7 +4691,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
FLEX_ALLOC_STR(entry, string, buf.buf);
oidcpy(&entry->entry.oid, &commit->object.oid);
- oidmap_put(&commit2todo, entry);
+ oidmap_put_entry(entry, entry, /* member name */
+ &commit2todo);
continue;
}
@@ -4731,7 +4734,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
FLEX_ALLOC_STR(entry, string, buf.buf);
oidcpy(&entry->entry.oid, &commit->object.oid);
- oidmap_put(&commit2todo, entry);
+ oidmap_put_entry(entry, entry, /* member name */
+ &commit2todo);
}
/*
@@ -4769,7 +4773,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
commit = iter->item;
if (oidset_contains(&shown, &commit->object.oid))
continue;
- entry = oidmap_get(&state.commit2label, &commit->object.oid);
+
+ oidmap_get_entry(entry, entry /* member name */,
+ &state.commit2label, &commit->object.oid);
if (entry)
strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
@@ -4793,8 +4799,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
else {
const char *to = NULL;
- entry = oidmap_get(&state.commit2label,
- &commit->object.oid);
+ oidmap_get_entry(entry, entry /* member name */,
+ &state.commit2label, &commit->object.oid);
if (entry)
to = entry->string;
else if (!rebase_cousins)
@@ -4813,11 +4819,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,
for (iter2 = list; iter2; iter2 = iter2->next) {
struct object_id *oid = &iter2->item->object.oid;
- entry = oidmap_get(&commit2todo, oid);
+ oidmap_get_entry(entry, entry /* member name */,
+ &commit2todo, oid);
/* only show if not already upstream */
if (entry)
strbuf_addf(out, "%s\n", entry->string);
- entry = oidmap_get(&state.commit2label, oid);
+ oidmap_get_entry(entry, entry /* member name */,
+ &state.commit2label, oid);
if (entry)
strbuf_addf(out, "%s %s\n",
cmd_label, entry->string);
diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c
index 3f599b21b8..fc19c1cb8e 100644
--- a/t/helper/test-oidmap.c
+++ b/t/helper/test-oidmap.c
@@ -59,7 +59,7 @@ int cmd__oidmap(int argc, const char **argv)
oidcpy(&entry->entry.oid, &oid);
/* add / replace entry */
- entry = oidmap_put(&map, entry);
+ entry = oidmap_put_entry(entry, entry, /* member name */ &map);
/* print and free replaced entry, if any */
puts(entry ? entry->name : "NULL");
@@ -73,7 +73,8 @@ int cmd__oidmap(int argc, const char **argv)
}
/* lookup entry in oidmap */
- entry = oidmap_get(&map, &oid);
+ oidmap_get_entry(entry, entry /* member name */,
+ &map, &oid);
/* print result */
puts(entry ? entry->name : "NULL");
@@ -86,7 +87,8 @@ int cmd__oidmap(int argc, const char **argv)
}
/* remove entry from oidmap */
- entry = oidmap_remove(&map, &oid);
+ oidmap_remove_entry(entry, entry, /* member name */
+ &map, &oid);
/* print result and free entry*/
puts(entry ? entry->name : "NULL");
--
2.26.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-25 3:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar
2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar
2020-04-08 6:02 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Junio C Hamano
2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar
2020-04-08 7:03 ` [PATCH v2 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar
2020-04-08 22:08 ` Jeff King
2020-04-08 21:54 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Jeff King
2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar
2020-04-25 2:59 ` [PATCH v3 2/3] oidmap: rework iterators to return typed pointer Abhishek Kumar
2020-04-25 2:59 ` [PATCH v3 3/3] oidmap: return oidmap_entry pointers Abhishek Kumar
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).