On Thu, 2025-02-20 at 12:18 -0600, Rob Browning wrote: > "Dr. Arne Babenhauserheide" <arne_...@web.de> 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).
This should be the case, I always make sure that all intermediate states compile. If I missed something, please let me know where you see a failure. > - 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. Shorter, more maintainable code is IMHO always a win. If you insist, I can add more details why it's better to call scm_to_mpz that internally uses either mpz_init_set_si or scm_integer_init_set_mpz_z, depending on whether the argument is an immediate. > - 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". There are plenty SCM_INTERNAL internal functions in libguile. In this particular case, scm_from_inum is static so it will never be visible outside of its translation unit. > - 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'm not sure how this is relevant. If you have a concrete item to look at, please let me know. > - 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. I don't think there is, but otherwise such platforms would be equally broken as Windows because current Guile *assumes* that hashes, currently stored as long, are the same size as pointers. If that's not the case, it won't even boot. > - 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 According to my analysis, the current code assumes hashes are the same size as pointers. Maybe this could be changed, but this would mean much larger changes and at least incompatible bytecode if not API. > - 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. I'm not sure where exactly this applies... > And note that I didn't look carefully at what the main code was actually > *doing* at all. > > Hope this helps If I'm honest, it would help more to get reviews from people actually looking at the code...
signature.asc
Description: This is a digitally signed message part
