Patrick Steinhardt <ps@xxxxxx> writes:
> For C programs:
>
> + - We use `size_t` to count the number of bytes and `count_t` to count the
> + number of entities of a given type.
I am not interested in this specific implementation at all for a
number of reasons, but I am excited to see people thinking about the
issues. The following is a random list of things, both positive and
negative, that came to my mind after skimming the changes.
* We do not want to pretend that one size fits all. If it were a
good idea for developers to express "This variable is a simple
counter that counts up from 0 and never goes negative" by using
an unsigned type (which is dubious), it should be equally, or not
more, a good idea to allow them to say "We will not have more
than 256 fan-out directories under .git/objects/ and this is a
counter to count them, so I know 'unsigned short' is big enough
on any platforms".
* As far as I can tell, the patch does not seem to address the
biggest concern of unsigned integer wraparound. We often see
ALLOC_GROW(thing.entry, thing.nr + 1, thing.alloc);
with the arithmetic "thing.nr + 1" checked by nobody.
ALLOC_GROW_BY() is slightly better in this regard, but nobody
uses it with only small exceptions. And of course, alloc_nr()
does even riskier arithmetic that is unchecked.
* Standardising the names used for <item[], item_nr, item_alloc>
somehow is very much welcome (we can see an example in the change
to builtin/rm.c below). Such a naming convention would allow us
to write
#define ALLOC_INCR(thing) ALLOC_INCR_BY(thing, 1)
ALLOC_INCR_BY(thing, increment)
that do ALLOC_GROW(thing, thing_nr + increment, thing_alloc) more
safely than what the current code does, perhaps? Also, we should
be able to use any unsigned integral type and perform sensible
bound checking with typeof().
* The codebase avoids inventing a new type with typedef, with the
exception of callback function type, following old tradition we
inherited from the Linux kernel project. And even when we create
a new type, of course, we do not want to give it a name that ends
with "_t".
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 05d89e98c3..99b845cf34 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -33,11 +33,11 @@ static const char * const builtin_rm_usage[] = {
> };
>
> static struct {
> - int nr, alloc;
> struct {
> const char *name;
> char is_submodule;
> } *entry;
> + count_t entry_nr, entry_alloc;
> } list;
