"Dr. Arne Babenhauserheide" <[email protected]> writes:
> I tried to ping in IRC again, but since no one seems fit to do the
> reviews, I put it on my own TODO list.
So this belongs in the category of somewhat cursory comments, and I'm
not sure I should be the more substantive reviewer here, and/or have
the time to remember the relevant information (say with respect to gmp)
to make that feasible right now, but from a partial look:
- If the patches are introducing code that uses a type before the type
itself (order-wise), then I'd favor reordering the patches so that
the build is never broken (e.g. for git bisect).
- If not resolved by a reordering, it'd be nice to include a "why" in
the commit messages where the change doesn't have clear context (for
more casual reviewers, or later code archaeology), e.g. why does the
first commit move to scm_to_mpz.
- Regarding renaming of static (internal) functions to scm_*,
e.g. scm_from_inum, I wasn't sure whether we intended to limit that
prefix to public functions. I'd tended to assume "perhaps".
- If I recall correctly, in the past some of the numeric code
intentionally used more complicated (if/then) logic to allow it to
call more specific functions in each case in order to avoid the
costs of more general calls (having to redo common dispatch, etc.).
I wondered in passing if that might be relevant here, but I haven't
worked with gmp in any detail in a good while.
- I also wondered if there are any existing platforms where
sizeof(intptr_t) != sizeof(long), meaning (I think) that this would
be a backward incompatible change (ABI break) -- and without further
careful thought/investigation to convince myself it's fine, I'd be
wary of changing the hash type in a Z release.
- Ignoring the compatibility question, and more generally (longer
term), I also wondered about the fundamental intent of the hash type
if we're thinking of changing it. Is it supposed to be the fastest
"bigger integer" on the platform (suggesting "long"), or is it
supposed to always be 32 or 64 bit (suggesting uint32_t or
uint64_t), or is it always supposed to be "pointer sized",
suggesting uintptr_t, though that might not be quite right, since I
did look briefly at a POSIX spec and didn't see any restrictions on
the relative sizes of uintptr_t/intptr_t with respect to anything
else, excepting that they be big enough for pointers, but of course
could be bigger.
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html
- If the code is packing two 32-bit integers into a(n assumed at
least) 64-bit integer, then the C compiler's probably not going to
complain about unintentional uses of that packed value as a single
integer. If there are any concerns that might cause us to miss
something important, I wondered if there would be any point, if it's
feasible, to define it as a non-integral type, even if just
temporarily, to force the compiler to expose any such situations.
And note that I didn't look carefully at what the main code was actually
*doing* at all.
Hope this helps
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4
