Jeff King <peff@xxxxxxxx> writes: > 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). > But isn't that the case now anyways? We always lose the const-ness since we only accept a 'void *'. But by only changing 'strmap_put()' to accept a 'const void *', but storing and returning a 'void *'. We simply modify the current construct to also say that any data received is not modified. But I do see your point, we'll have to cast there anyways and might as well do it on the client side. > 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 Agreed, and then there is additional load to ensure users will use 'strmap_put()' and 'strmap_put_const()' as required and simply not cast away. Overall I agree with what you're saying. Thanks for spelling it out. Karthik
Attachment:
signature.asc
Description: PGP signature
