Re: [PATCH v5 09/11] pack-objects: shrink size field in struct object_entry

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> +static inline int contains_in_32bits(unsigned long limit)
> +{

This name somehow does not sound right.

If the verb "contain" must be used, the way to phrase what this
function does with the verb is to say "limit can be contained in a
32-bit int", so "contains" is probably where the funniness comes
from.

"fits in 32bits" is OK, I think.

> +	uint32_t truncated_limit = (uint32_t)limit;
> +
> +	return limit == truncated_limit;
> +}

I am guessing that a compiler that is clever enough will make this
function a no-op on a 32-bit arch and that is why it is a static
inline function?

> +static inline int oe_size_less_than(const struct object_entry *e,
> +				    unsigned long limit)
> +{
> +	if (e->size_valid)
> +		return e->size_ < limit;

e->size_ is the true size so we can compare it to see if it is smaller
than limit.

> +	if (contains_in_32bits(limit))
> +		return 1;

If limit is small enough, and because e->size_valid means e->size_
does not fit in 32-bit, we know size is larger than limit.
Shouldn't we be returning 0 that means "no, the size is not less
than limit" from here?

> +	return oe_size(e) < limit;
> +}
> +
> +static inline int oe_size_greater_than(const struct object_entry *e,
> +				       unsigned long limit)
> +{
> +	if (e->size_valid)
> +		return e->size_ > limit;

e->size_ is the true size so we compare and return if it is larger
than limit.

> +	if (contains_in_32bits(limit))
> +		return 0;

Now e->size_ is larger than what would fit within 32-bit.  If limit
fits within 32-bit, then size must be larger than limit.  Again,
shouldn't we be returning 1 that means "yes, the size is greater
than limit" from here?

> +	return oe_size(e) > limit;
> +}
> +
> +static inline void oe_set_size(struct object_entry *e,
> +			       unsigned long size)
> +{
> +	e->size_ = size;
> +	e->size_valid = e->size_ == size;
> +}
> +
>  #endif