Re: [HACKERS] Function structure in formatting.c

Hi hackers,

I'm currently poking around in backend/utils/adt/formatting.c with a
view to improving to_date() parsing (see thread at
http://archives.postgresql.org/pgsql-hackers/2007-07/msg00513.php),
and I've noticed that the way the functions are organised is pretty
weird.

The original author appears to have gone to great lengths to make the
various functions work both for conversions *to* string, and *from*
string. For each formatting "keyword" (DD, MM, etc), there is just
one processing function; dch_global, dch_date or dch_time. Each of
these takes an argument called "is_to_char". Since parsing a date out
of a string, and formatting a date into a string, are fundamentally
different objectives the functions end up reading a lot like this:

if (is_to_char)
{
// do something
}
else
{
// do something completely different
}

In fact, almost all of the actual formatting code in the file is
enclosed in one of these if .. else blocks.

To my mind, it would make a lot more sense (and make hacking the file
a lot easier) if the processing functions were split into to_char and
from_char variants. I'm not sure what, if any, advantage is gleaned
by having these functions combined.

I'd like to hear from someone who has more familiarity with
formatting.c on this. Is there some good reason for keeping the
functions unified?

Obviously there's a fair bit of work in splitting the functions up,
but I'd be willing to do it if only to spare my own sanity when
working on to_date parsing.

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> To my mind, it would make a lot more sense (and make hacking the file
> a lot easier) if the processing functions were split into to_char and
> from_char variants. I'm not sure what, if any, advantage is gleaned
> by having these functions combined.

Yeah, I never liked that approach either. I suppose the idea was to
keep the to- and from- code for each format code close together, but
it blurs what's going on to a much greater extent than that's worth.

regards, tom lane

On 8/9/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> > To my mind, it would make a lot more sense (and make hacking the file
> > a lot easier) if the processing functions were split into to_char and
> > from_char variants. I'm not sure what, if any, advantage is gleaned
> > by having these functions combined.
>
> Yeah, I never liked that approach either. I suppose the idea was to
> keep the to- and from- code for each format code close together, but
> it blurs what's going on to a much greater extent than that's worth.

Okay, I'll see what I can do.

Incidentally, anybody know what DCH is supposed to stand for?
Throughout the code DCH is used to refer to date/time formatting and
NUM to numeric formatting. Just curious.

Just quick update on this. It turns out (as it always does) that I
want to refactor a bit more intensively than I first suggested.

The functions dch_global, dch_time and dch_date seem to be wholly
pointless, since dch_global is effectively a one-liner for the FX
flag, and the other two merely wrap around a giant switch statement.

My thought was to split DCH_processor into a to_char flavour and a
from_char flavour, and have those functions do all the work; loop
through each of the FormatNodes and run the giant switch statement.

This means there is no need to put pointers to "action functions" in
each of the KeyWords. It makes the code simpler, more readable -- and
considerably shorter.

A patch will be on the way shortly.

On 8/9/07, Brendan Jurd <direvus(at)gmail(dot)com> wrote:
> Just quick update on this. It turns out (as it always does) that I
> want to refactor a bit more intensively than I first suggested.
>
[...]
> This means there is no need to put pointers to "action functions" in
> each of the KeyWords. It makes the code simpler, more readable -- and
> considerably shorter.
>
> A patch will be on the way shortly.
>

take your time, this seems like it will be for 8.4 anyway

--
regards,
Jaime Casanova

"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning."
Richard Cook

On 8/9/07, Jaime Casanova <systemguards(at)gmail(dot)com> wrote:
>
> take your time, this seems like it will be for 8.4 anyway
>

I hear you, unfortunately "taking my time" usually means I forget
about it for eight months and by the time I come back to it I've
forgotten what I was doing =)

I wasn't really expecting this to make it into 8.3. I just need to
get it done so I can free up the headspace for other projects.

Hello,

As discussed on -hackers, I've done some refactoring work on
backend/utils/adt/formatting.c, in an attempt to make the code a bit
more intelligible before improving handling of bogus formats.

This is purely a refactor. The functionality of the file hasn't
changed; it does the same job as before, but it does it in ~200 fewer
lines and ~3.5k fewer characters. The clarity of code is greatly
improved. Sadly, performance appears to be unchanged.

Summary of changes:

* Did away with dch_global, dch_date and dch_time.
* Replaced DCH_processor with two new functions DCH_to_char and
DCH_from_char, which now do all the work previously done by
dch_{global,date,time}.
* Removed the 'action' field from the KeyWord struct as it is no longer useful.
* Changed the type of the 'character' field in the FormatNode struct
to char, because ... that's what it is. The original choice of 'int'
seems to have been an error.
* Removed commented-out function declaration for is_acdc. According
to CVS annotate, this hasn't been in use since sometime in the early
Cretaceous period, and in any case I don't know why you'd want to
check whether a string was the rock band AC/DC. =)
* Reworded some of the comments for clarity.
* Didn't touch any of the number formatting routines.

This compiles cleanly on x86 gentoo and passes check, installcheck and
installcheck-parallel.

Thanks for your time,
BJ

This has been saved for the 8.4 release:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Brendan Jurd wrote:
> Hello,
>
> As discussed on -hackers, I've done some refactoring work on
> backend/utils/adt/formatting.c, in an attempt to make the code a bit
> more intelligible before improving handling of bogus formats.
>
> This is purely a refactor. The functionality of the file hasn't
> changed; it does the same job as before, but it does it in ~200 fewer
> lines and ~3.5k fewer characters. The clarity of code is greatly
> improved. Sadly, performance appears to be unchanged.
>
> Summary of changes:
>
> * Did away with dch_global, dch_date and dch_time.
> * Replaced DCH_processor with two new functions DCH_to_char and
> DCH_from_char, which now do all the work previously done by
> dch_{global,date,time}.
> * Removed the 'action' field from the KeyWord struct as it is no longer useful.
> * Changed the type of the 'character' field in the FormatNode struct
> to char, because ... that's what it is. The original choice of 'int'
> seems to have been an error.
> * Removed commented-out function declaration for is_acdc. According
> to CVS annotate, this hasn't been in use since sometime in the early
> Cretaceous period, and in any case I don't know why you'd want to
> check whether a string was the rock band AC/DC. =)
> * Reworded some of the comments for clarity.
> * Didn't touch any of the number formatting routines.
>
> This compiles cleanly on x86 gentoo and passes check, installcheck and
> installcheck-parallel.
>
> Thanks for your time,
> BJ

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

I noticed an editing error in the patch I originally submitted; it
defined the same debugging macro twice.

I've attached a fresh copy of the patch against the current HEAD with
the fix included.

Cheers,
BJ

On 8/11/07, Brendan Jurd <direvus(at)gmail(dot)com> wrote:
> Hello,
>
> As discussed on -hackers, I've done some refactoring work on
> backend/utils/adt/formatting.c, in an attempt to make the code a bit
> more intelligible before improving handling of bogus formats.
>
> This is purely a refactor. The functionality of the file hasn't
> changed; it does the same job as before, but it does it in ~200 fewer
> lines and ~3.5k fewer characters. The clarity of code is greatly
> improved. Sadly, performance appears to be unchanged.
>
> Summary of changes:
>
> * Did away with dch_global, dch_date and dch_time.
> * Replaced DCH_processor with two new functions DCH_to_char and
> DCH_from_char, which now do all the work previously done by
> dch_{global,date,time}.
> * Removed the 'action' field from the KeyWord struct as it is no longer useful.
> * Changed the type of the 'character' field in the FormatNode struct
> to char, because ... that's what it is. The original choice of 'int'
> seems to have been an error.
> * Removed commented-out function declaration for is_acdc. According
> to CVS annotate, this hasn't been in use since sometime in the early
> Cretaceous period, and in any case I don't know why you'd want to
> check whether a string was the rock band AC/DC. =)
> * Reworded some of the comments for clarity.
> * Didn't touch any of the number formatting routines.
>
> This compiles cleanly on x86 gentoo and passes check, installcheck and
> installcheck-parallel.
>
> Thanks for your time,
> BJ
>
>

This has been saved for the 8.4 release:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Brendan Jurd wrote:
> I noticed an editing error in the patch I originally submitted; it
> defined the same debugging macro twice.
>
> I've attached a fresh copy of the patch against the current HEAD with
> the fix included.
>
> Cheers,
> BJ
>
> On 8/11/07, Brendan Jurd <direvus(at)gmail(dot)com> wrote:
> > Hello,
> >
> > As discussed on -hackers, I've done some refactoring work on
> > backend/utils/adt/formatting.c, in an attempt to make the code a bit
> > more intelligible before improving handling of bogus formats.
> >
> > This is purely a refactor. The functionality of the file hasn't
> > changed; it does the same job as before, but it does it in ~200 fewer
> > lines and ~3.5k fewer characters. The clarity of code is greatly
> > improved. Sadly, performance appears to be unchanged.
> >
> > Summary of changes:
> >
> > * Did away with dch_global, dch_date and dch_time.
> > * Replaced DCH_processor with two new functions DCH_to_char and
> > DCH_from_char, which now do all the work previously done by
> > dch_{global,date,time}.
> > * Removed the 'action' field from the KeyWord struct as it is no longer useful.
> > * Changed the type of the 'character' field in the FormatNode struct
> > to char, because ... that's what it is. The original choice of 'int'
> > seems to have been an error.
> > * Removed commented-out function declaration for is_acdc. According
> > to CVS annotate, this hasn't been in use since sometime in the early
> > Cretaceous period, and in any case I don't know why you'd want to
> > check whether a string was the rock band AC/DC. =)
> > * Reworded some of the comments for clarity.
> > * Didn't touch any of the number formatting routines.
> >
> > This compiles cleanly on x86 gentoo and passes check, installcheck and
> > installcheck-parallel.
> >
> > Thanks for your time,
> > BJ
> >
> >

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: You can help support the PostgreSQL project by donating at
>
> http://www.postgresql.org/about/donate

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> I noticed an editing error in the patch I originally submitted; it
> defined the same debugging macro twice.
> I've attached a fresh copy of the patch against the current HEAD with
> the fix included.

Working through this patch now. I found one thing that seems to be
a mistake (probably an overenthusiastic search&replace): the patch
changes
- {"iy", 2, dch_date, DCH_IY, TRUE},
to
+ {"iyear", 2, DCH_IY, TRUE},

The removal of dch_date is intended, but surely the keyword should
still be "iy". I'm proceeding on that assumption, but if this change
was actually intended, please explain.

regards, tom lane

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
>> As discussed on -hackers, I've done some refactoring work on
>> backend/utils/adt/formatting.c, in an attempt to make the code a bit
>> more intelligible before improving handling of bogus formats.

Applied with minor revisions.

regards, tom lane

On 23/03/2008, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Working through this patch now. I found one thing that seems to be
> a mistake (probably an overenthusiastic search&replace): the patch
> changes
> - {"iy", 2, dch_date, DCH_IY, TRUE},
> to
> + {"iyear", 2, DCH_IY, TRUE},
>
> The removal of dch_date is intended, but surely the keyword should
> still be "iy". I'm proceeding on that assumption, but if this change
> was actually intended, please explain.
>

Nice catch. Not sure how that got in there, but your theory about a
search & replace gone awry seems the most likely.

Now that the functions have been refactored, I'm looking forward to
getting back into improving the sanity checking in to_date.

Cheers,
BJ