Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
On 12/22/2017 12:16 PM, Brandon Williams wrote:
On 12/22, Johannes Schindelin wrote:
[...]
That pattern is violated by OIDMAP_INIT, though. The first call to
oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
design: The underlying hashmap is not initialized correctly, as the
cmpfn function pointer still points to NULL, but since there are no
entries to be compared, cmpfn will not be called. Things break down,
though, as soon as there is even one entry.
Rather than causing confusion, frustration and needless loss of time due
to pointless debugging, let's *rename* OIDMAP_INIT so that developers
who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
that with oidmaps.
-#define OIDMAP_INIT { { NULL } }
+/*
+ * This macro initializes the data structure only incompletely, just enough
+ * to call oidmap_get() on an empty map. Use oidmap_init() instead.
+ */
+#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
Alternatively, could we define the macro to an expression
that would cause a compiler error? Then any new code written
would fail to compile. And we document the issue in a comment
above the macro so no one changes the macro to "fix" it.
(I suggest this as opposed to simply removing the macro
to prevent someone from re-adding it later, since it is the
standard pattern.)
Just a thought,
Jeff
Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
Hi Junio,
On Fri, 22 Dec 2017, Junio C Hamano wrote:
> Brandon Williams writes:
>
> >> -#define OIDMAP_INIT { { NULL } }
> >> +/*
> >> + * This macro initializes the data structure only incompletely, just
> >> enough
> >> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
> >> + */
> >> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
> >
> > This is one way of approaching the problem. Couldn't we also take the
> > approach like we have with oidset and ensure that when oidmap_get() or
> > _put() are called that if the oidmap isn't initialized, we simply do
> > that then?
>
> Hmph. Can you show how the alternative code would look like?
No, because I refuse to perform pointless work, in particular when I am
already pretty booked with work.
But you know how it would look like, right? The cmpfn() function would be
exported via oidmap.h, and a HASHMAP_INIT(cmpfn) would be introduced in
hashmap.h that would initialize everything zeroed out except for the
cmpfn.
But then you would review it and ask if there would be any use in adding
cmp_cb_data to the signature of the HASHMAP_INIT() macro, and I would have
to implement that, too.
And then nobody would use it, and the macro would very likely get
stale/incorrect. And then I would offer another patch reverting that
change (because there is no user) and replace it with this here patch.
As I said: pointless,
Dscho
Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
Brandon Williams writes:
>> -#define OIDMAP_INIT { { NULL } }
>> +/*
>> + * This macro initializes the data structure only incompletely, just enough
>> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
>> + */
>> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
>
> This is one way of approaching the problem. Couldn't we also take the
> approach like we have with oidset and ensure that when oidmap_get() or
> _put() are called that if the oidmap isn't initialized, we simply do
> that then?
Hmph. Can you show how the alternative code would look like?
Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
On 12/22, Johannes Schindelin wrote:
> In Git's source code, we have this convention that quite a few data
> structures can be initialized using a macro *_INIT while defining an
> instance (instead of having to call a function to initialize the data
> structure). You will see that idiom quite a bit, e.g. `struct strbuf buf
> = STRBUF_INIT;`
>
> This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is
> perfectly legal and you can use that data structure right away, without
> having to call `oidset_init()` first.
>
> That pattern is violated by OIDMAP_INIT, though. The first call to
> oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
> design: The underlying hashmap is not initialized correctly, as the
> cmpfn function pointer still points to NULL, but since there are no
> entries to be compared, cmpfn will not be called. Things break down,
> though, as soon as there is even one entry.
>
> Rather than causing confusion, frustration and needless loss of time due
> to pointless debugging, let's *rename* OIDMAP_INIT so that developers
> who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
> that with oidmaps.
>
> An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that
> in oidmap.h. However, there are *no* call sites in Git's source code
> that would benefit from that macro, i.e. this would be premature
> optimization (and cost a lot more time than this here trivial patch).
>
> Signed-off-by: Johannes Schindelin
> ---
> Published-As:
> https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1
> oidmap.h | 6 +-
> oidset.h | 7 ++-
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/oidmap.h b/oidmap.h
> index 18f54cde143..1a73c392b79 100644
> --- a/oidmap.h
> +++ b/oidmap.h
> @@ -21,7 +21,11 @@ struct oidmap {
> struct hashmap map;
> };
>
> -#define OIDMAP_INIT { { NULL } }
> +/*
> + * This macro initializes the data structure only incompletely, just enough
> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
> + */
> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
This is one way of approaching the problem. Couldn't we also take the
approach like we have with oidset and ensure that when oidmap_get() or
_put() are called that if the oidmap isn't initialized, we simply do
that then?
>
> /*
> * Initializes an oidmap structure.
> diff --git a/oidset.h b/oidset.h
> index f4c9e0f9c04..a11d88edc1d 100644
> --- a/oidset.h
> +++ b/oidset.h
> @@ -22,7 +22,12 @@ struct oidset {
> struct oidmap map;
> };
>
> -#define OIDSET_INIT { OIDMAP_INIT }
> +/*
> + * It is okay to initialize the map incompletely here because oidset_insert()
> + * will call oidset_init() (which will call oidmap_init()), and
> + * oidset_contains() works as intended even before oidset_init() was called.
> + */
> +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }
>
> /**
> * Returns true iff `set` contains `oid`.
>
> base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279
> --
> 2.15.1.windows.2
--
Brandon Williams
[PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
In Git's source code, we have this convention that quite a few data
structures can be initialized using a macro *_INIT while defining an
instance (instead of having to call a function to initialize the data
structure). You will see that idiom quite a bit, e.g. `struct strbuf buf
= STRBUF_INIT;`
This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is
perfectly legal and you can use that data structure right away, without
having to call `oidset_init()` first.
That pattern is violated by OIDMAP_INIT, though. The first call to
oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
design: The underlying hashmap is not initialized correctly, as the
cmpfn function pointer still points to NULL, but since there are no
entries to be compared, cmpfn will not be called. Things break down,
though, as soon as there is even one entry.
Rather than causing confusion, frustration and needless loss of time due
to pointless debugging, let's *rename* OIDMAP_INIT so that developers
who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
that with oidmaps.
An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that
in oidmap.h. However, there are *no* call sites in Git's source code
that would benefit from that macro, i.e. this would be premature
optimization (and cost a lot more time than this here trivial patch).
Signed-off-by: Johannes Schindelin
---
Published-As:
https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1
Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1
oidmap.h | 6 +-
oidset.h | 7 ++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/oidmap.h b/oidmap.h
index 18f54cde143..1a73c392b79 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -21,7 +21,11 @@ struct oidmap {
struct hashmap map;
};
-#define OIDMAP_INIT { { NULL } }
+/*
+ * This macro initializes the data structure only incompletely, just enough
+ * to call oidmap_get() on an empty map. Use oidmap_init() instead.
+ */
+#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
/*
* Initializes an oidmap structure.
diff --git a/oidset.h b/oidset.h
index f4c9e0f9c04..a11d88edc1d 100644
--- a/oidset.h
+++ b/oidset.h
@@ -22,7 +22,12 @@ struct oidset {
struct oidmap map;
};
-#define OIDSET_INIT { OIDMAP_INIT }
+/*
+ * It is okay to initialize the map incompletely here because oidset_insert()
+ * will call oidset_init() (which will call oidmap_init()), and
+ * oidset_contains() works as intended even before oidset_init() was called.
+ */
+#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }
/**
* Returns true iff `set` contains `oid`.
base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279
--
2.15.1.windows.2
