On Tue, May 20, 2025 at 02:05:09AM -0700, Karthik Nayak wrote:
> > Not an issue with this series at all, but one thing I wondered is if
> > it makes sense to change the type of strmap_get/strmap_put to deal
> > with "const void *". That way, it would not be necessary to cast
> > away the constness like so:
> >
> >> -+ strmap_put(failed_refs, refname, ref_transaction_error_msg(err));
> >> ++ strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
> >
> > without harming the other side, namely
> >
> >> @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com
> >> + if (reported_error)
> >> + cmd->error_string = reported_error;
> >> + else if (strmap_contains(&failed_refs, cmd->ref_name))
> >> -+ cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
> >> ++ cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
> >
> > this piece of code.
> >
> > It may not make sense, and even if it did, of course, it is totally
> > outside of this series.
> >
> > Thanks.
>
> It definitely does, The only other typecast I did find for `strmap_put`
> was within 'strmap.h'. Nevertheless, I think it makes sense to make that
> change. strmap shouldn't modify the data provided. Perhaps #leftoverbits.
I'm not sure that is a good idea. Even though strmap does not touch the
void data pointer itself, it is accessible to the callers, and we do not
know if they stored const data or not, or how they plan to access it.
If we store a "const void *" pointer and returned that via strmap_get(),
then there will be callers who need to cast away the constness.
If we store and return a "void *" pointer as we do now, but accept a
const pointer via strmap_put(), then we're casting away potentially
important const-ness without the caller even seeing it. I think it's
safer for the client to do the cast explicitly (since they are the ones
who know how they plan to use it).
I don't think we can really represent what we want in C's type system.
But if we wanted a safe(r) interface that didn't involve type-casting,
we might be able to do something like:
- the strmap stores an extra flag for "the data pointer is const",
which can be set by strmap_init(). (It is tempting to replace
strmap_clear()'s free_entries parameter by checking this flag, but
the two are not always going to be exactly the same).
- introduce strmap_get_const() and strmap_put_const() which give and
receive const pointers
- in the const functions, BUG() if the "pointers are const" flag is
not set
But it feels gross, and it only gives runtime checking, not
compile-time. What we really want are generics that can carry the type
annotation for a particular instantiated map. That is probably pretty
easy in most modern languages, but not really in C without horrible
macros. :)
-Peff
