Re: [PATCH 1/3] date: add helpers to convert between "+HHMM" timezones and minutes

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

Karthik Nayak <karthik.188@gmail.com> writes:

>> -	int minutes;
>> +	int minutes = tz < 0 ? -tz : tz;
>
> This is the part which we could skip as we're C99 compliant, but keeping
> to be on the safe side.
>
>> +	minutes = (minutes / 100) * 60 + (minutes % 100);
>> +	return tz < 0 ? -minutes : minutes;
>> +}

I was wondering exactly the same thing yesterday.

As written, it is clear even to those unfamiliar with the C89/C99
signed division rules, because we deal only with non-negative
numbers, which is a plus.  The fewer things readers need to worry
about, the better.


>>  	offset /= 60; /* in minutes */
>> -	offset = (offset % 60) + ((offset / 60) * 100);
>> -	return offset * eastwest;
>> +	return minutes_to_tz(offset * eastwest);
>
> While mathematically it's the same, but shouldn't this have been
> `minutes_to_tz(offset) * eastwest`?

The way you suggest is more faithful rewrite of the original.

Thanks.