On Tue, Nov 3, 2020 at 8:20 AM Jeff King <peff@xxxxxxxx> wrote:
>
> On Fri, Oct 30, 2020 at 10:28:51AM -0700, Elijah Newren wrote:
>
> > > You might want to mention that this _could_ be done as just accessors to
> > > strmap, but using a separate struct provides type safety against
> > > misusing pointers as integers or vice versa.
> >
> > If I just did it as accessors, it makes it harder for myself and
> > others to remember what my huge piles of strmaps in merge-ort do; I
> > found that it became easier to follow the code and remember what
> > things were doing when some were marked as strmap, some as strintmap,
> > and some as strset.
>
> Oh, I'm definitely on board with that argument. I was just suggesting
> you might want to put it in the commit message for posterity.
>
> > > > +/*
> > > > + * strintmap:
> > > > + * A map of string -> int, typecasting the void* of strmap to an int.
> > >
> > > Are the size and signedness of an int flexible enough for all uses?
> >
> > If some users want signed values and others want unsigned, I'm not
> > sure how we can satisfy both. Maybe make a struintmap?
>
> Right, that was sort of my question: do your users actually want it
> signed or not. Sounds like they do want it signed, and don't mind the
> loss of range.
>
> > Perhaps that could be added later if uses come up for it? Some of my
> > uses need int, the rest of them wouldn't care about int vs unsigned.
>
> Yeah, if you don't have any callers which care, I'd definitely punt on
> it for now.
>
> > If someone does care about the full range of bits up to 64 on relevant
> > platforms, I guess I should make it strintptr_t_map.
>
> Yeah, that's what I was wondering. I suspect the use case for that is
> pretty narrow, though. If you really care about having a 64-bit value
> for some data, then you probably want it _everywhere_, not just on
> 64-bit platforms. I guess the exception would be if you're mapping into
> size_t's or something.
>
> I think my question was as much "did you think about range issues for
> your intended users" as "should we provide more range in this map type".
> And it sounds like you have thought about that, so I'm happy proceeding.
>
> > But besides the
> > egregiously ugly name, one advantage of int over intptr_t (or unsigned
> > over uintptr_t) is that you can use it in a printf easily:
> > printf("Size: %d\n", strintmap_get(&foo, 0));
> > whereas if it strintmap_get() returns an intptr_t, then it's a royal
> > mess to attempt to portably use it without adding additional manual
> > casts. Maybe I was just missing something obvious, but I couldn't
> > figure out the %d, %ld, %lld, PRIdMAX, etc. choices and get the
> > statement to compile on all platforms, so I'd always just cast to int
> > or unsigned at the time of calling printf.
>
> The right way is:
>
> printf("Size: %"PRIdMAX", (intmax_t) your_intptr_t);
Ah, intmax_t; that's what I was missing.
> which will always do the right thing no matter the size (at the minor
> cost of passing a larger-than-necessary parameter, but if you're
> micro-optimizing then calling printf at all is probably already a
> problem).
>
> But yeah, in general using a real "int" is much more convenient and if
> there's no reason to avoid it for range problems, I think it's
> preferable.
Yep, I like the simplicity of "int", the signedness of "int" and it
has far more than enough range on all platforms (most my strintmaps
actually map to enum values, but my largest int usage is for counting
up to at most how many files are involved in rename detection. Even
microsoft repos only have a number of files present in the repository
that registers in the low millions, and I'm only dealing with the
subset of those files involved in rename detection, which should be
much smaller).
