Re: [PATCH v2 2/6] midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



William Baker <williamtbakeremail@xxxxxxxxx> writes:

> On 9/20/19 1:10 PM, Junio C Hamano wrote:
>>> diff --git a/midx.c b/midx.c
>>> index b2673f52e8..54e4e93b2b 100644
>>> --- a/midx.c
>>> +++ b/midx.c
>>> @@ -449,6 +449,8 @@ struct pack_list {
>>>  	uint32_t nr;
>>>  	uint32_t alloc;
>>>  	struct multi_pack_index *m;
>>> +	struct progress *progress;
>>> +	uint32_t pack_paths_checked;
>>>  };
>> 
>> What is the reason behind u32 here?  Do we want to be consistently
>> limited to 4G on all platforms?
>> 
>> I have a feeling that we do not care too deeply all that much for
>> the purpose of progress output, in which case, just an ordinary
>> "unsigned" may be much less misleading.
>
> I went with u32 here to match the data type used to track how many
> entries are in the pack_list ('nr' is a u32).

That kind of parallel is valid when you could compare nr with this
new thing (or put it differently, the existing nr and this new thing
counts the same).  Are they both about the number of packs?

> I could switch to this to an unsigned but we would run the (extremely
> unlikely) risk of having more than 65k packs on a system where
> unsigned is 16 bits.

Why?  If an arch is small enough that the natural integer size is 16-bit,
then limiting the total number of packs to 65k sound entirely
sensible.

The only reason why you'd want fixed (across platforms and
architectures) type is when you want to make sure that a file
storing the literal values taken from these fields are portable and
everybody is limited the same way.  If a platform's natural integer
is 64-bit, by artificially limiting the size of this field to u32,
you are making disservice to the platform users, and more
importantly, you are wasting developers' time by forcing them to
wonder if there is a reason behind the choice of u32 (does it really
need to be able to store up to 4G, even on a smaller machines?  Is
it necessary to refuse to store more than 4G?  What are the
reasons?), like me wondering about these questions and writing them
down here.

So, unless there is a reason why this must be of fixed type, I'd say
just an unsigned would be the most reasonable choice.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux