Thread: Re: [HACKERS] Function structure in formatting.c
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
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@momjian.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@gmail.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
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@gmail.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@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
"Brendan Jurd" <direvus@gmail.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@gmail.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@sss.pgh.pa.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