Re: [PATCH] Don't truncate integer part in to_char for 'FM99.'

Marti Raudsepp <marti(at)juffo(dot)org> writes:
> On Wed, Sep 7, 2011 at 21:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Hmm. I agree that this is a bug, but the proposed fix seems like a bit
>> of a kluge. Wouldn't it be better to make get_last_relevant_decnum
>> honor its contract, that is not delete any relevant digits?

> You're right, it was a kludge.

> Here's an improved version. I need to take a NUMProc* argument to do
> that right, because that's how it knows how many '0's to keep from the
> format.

Yeah, after fooling with it myself I saw that it was the interconnection
of the don't-suppress-'0'-format-positions logic with the find-the-last-
nonzero-digit logic that was confusing matters. (formatting.c may not
be the most spaghetti-ish code I've ever worked with, but it's up
there.) However, I don't think that inserting knowledge of the other
consideration into get_last_relevant_decnum is really the way to make
things cleaner. Also, the way yours is set up, I'm dubious that it
does the right thing when the last '0' specifier is to the left of the
decimal point. I'm currently testing this patch:

diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 7efd988362889346af86c642f6ee660a4ae1b3d2..a7000250b0363165bee5697ad72036aad28b830e 100644
*** a/src/backend/utils/adt/formatting.c
--- b/src/backend/utils/adt/formatting.c
*************** NUM_prepare_locale(NUMProc *Np)
*** 3908,3913 ****
--- 3908,3916 ----
/* ----------
* Return pointer of last relevant number after decimal point
* 12.0500 --> last relevant is '5'
+ * 12.0000 --> last relevant is '.'
+ * If there is no decimal point, return NULL (which will result in same
+ * behavior as if FM hadn't been specified).
* ----------
*/
static char *
*************** get_last_relevant_decnum(char *num)
*** 3921,3927 ****
#endif

if (!p)
! p = num;
result = p;

while (*(++p))
--- 3924,3931 ----
#endif

if (!p)
! return NULL;
!
result = p;

while (*(++p))
*************** NUM_processor(FormatNode *node, NUMDesc
*** 4458,4470 ****
{
Np->num_pre = plen;

! if (IS_FILLMODE(Np->Num))
{
! if (IS_DECIMAL(Np->Num))
! Np->last_relevant = get_last_relevant_decnum(
! Np->number +
! ((Np->Num->zero_end - Np->num_pre > 0) ?
! Np->Num->zero_end - Np->num_pre : 0));
}

if (Np->sign_wrote == FALSE && Np->num_pre == 0)
--- 4462,4483 ----
{
Np->num_pre = plen;

! if (IS_FILLMODE(Np->Num) && IS_DECIMAL(Np->Num))
{
! Np->last_relevant = get_last_relevant_decnum(Np->number);
!
! /*
! * If any '0' specifiers are present, make sure we don't strip
! * those digits.
! */
! if (Np->last_relevant && Np->Num->zero_end > Np->num_pre)
! {
! char *last_zero;
!
! last_zero = Np->number + (Np->Num->zero_end - Np->num_pre);
! if (Np->last_relevant < last_zero)
! Np->last_relevant = last_zero;
! }
}

if (Np->sign_wrote == FALSE && Np->num_pre == 0)

regards, tom lane