On Thu, May 11, 2023 at 11:31 AM Jeff King <peff@xxxxxxxx> wrote:
>
> On Thu, May 11, 2023 at 05:24:01PM +0000, Calvin Wan wrote:
>
> > > Note that there's a small but
> > > important change to khash.h in this patch as well to allow this move,
> > > which is easy to overlook.
> >
> > Can you go into a bit more detail as to how this change allows the move?
> > (An example of the concatenation would probably be sufficient)
>
> Even after finding the hunk, I have to admit I scratched my head at what
> was going on.
>
> The answer is that object-store-ll.h now uses "struct kh_odb_path_map"
> instead of a typedef'd kh_odb_path_map. Using the typedef is the way
> that the khash library intends to work (and the only way it provides),
> but does not match our usual Git style. In other users of the khash
> library, we just bend to their will and use the typedef typedef (which
> we have to, because it does not even name the struct type directly!).
>
> But we can't do that here, because object-store-ll.h does not include
> khash.h, so must forward declare the struct (actually, we do not even do
> that, but it is legal to just refer to it as a pointer). But the
> compiler only understands what we are doing if the "struct" keyword is
> present.
>
> So I think the solution here is reasonable, but I actually think it
> would make sense to pull it out into its own patch, with a rationale
> like:
>
> khash.h lets you instantiate custom hash types that map between two
> types. These are defined as a struct, as you might expect, and khash
> typedef's that to kh_foo_t. But it declares the struct anonymously,
> which doesn't give a name to the struct type itself; there is no
> "struct kh_foo". This has two small downsides:
>
> - when using khash, we declare "kh_foo_t *the_foo". This is
> unlike our usual naming style, which is "struct kh_foo *the_foo".
>
> - you can't forward-declare a typedef of an unnamed struct type in
> C. So we might do something like this in a header file:
>
> struct kh_foo;
> struct bar {
> struct kh_foo *the_foo;
> };
>
> to avoid having to include the header that defines the real
> kh_foo. But that doesn't work with the typedef'd name. Without the
> "struct" keyword, the compiler doesn't know we mean that kh_foo is
> a type.
>
> So let's always give khash structs the name that matches our
> conventions ("struct kh_foo" to match "kh_foo_t"). We'll keep doing
> the typedef to retain compatibility with existing callers.
>
> The "you can't do this in C" is all off the top of my head. I think it's
> correct, but there may be a clever way to do it that I don't know of.
> Though even there is, I think "make khash more like our usual
> conventions" is a good thing by itself.
Oh, wow, I was about to type up an explanation to Calvin, but you did
so in even greater detail. Sweet.
And sure, I'd be happy to split this out into a separate patch and
steal the commit message. I think you deserve Co-author, though,
given it's only a 1-line code change versus the 20 line commit message
you wrote...
