Re: [PATCH] replace float8 with int in date2isoweek() and date2isoyear()

=?utf-8?q?=D0=A4=D1=83=D0=BA=D0=B0=D0=BD=D1=87=D0=B8=D0=BA_=D0=A1=D0=B5=D1=80=D0=B3=D0=B5=D0=B9?=
<s.fukanchik@postgrespro.ru>writes: 
> Hi PG hackers,
> I found suspicious use of float8 in date2isoweek() and date2isoyear(). In both
> cases float8 is only used for storing the value, while the entire calculation
> on the right happens in integers:

>  float8 result = (dayn - (day4 - day0)) / 7 + 1;

> At the end date2isoweek() returns `result' converted back to int:

>  return (int) result;

> float8 here is confusing and a bit slow.

I looked into our git history to try to find out why it's like this.
The answer seems to be that commit dffd8cac3 created date2isoweek()
by splitting out pre-existing code that had been in timestamp_part().
In that context the code had been using a float8 "result" variable
that was shared with other switch cases, and that variable's type
was just blindly copied into date2isoweek().  Then 1c757c49f again
copied-and-pasted while creating date2isoyear().

I agree with getting rid of the unnecessary usage of float8 here,
but there's another aspect that's bugging me: "result" is a totally
misleading variable name in date2isoyear(), because it's *not*
the function's result.  I'm inclined to rename it to "week", and
then to keep these functions looking as parallel as possible,
I'd probably do the same in date2isoweek().

> I think there is no need in adding an extra test case here, because
> date2isoweek and date2isoyear are covered by three regression tests:

Agreed, the code coverage report shows these are covered.

            regards, tom lane