Re: 1st patch for src/formats.c

[Jody Goldberg <jody gnome org>]

On Mon, Sep 23, 2002 at 10:07:10PM +0200, Nicolas Peninguy wrote:
> Hello everyone, hello Jody,
>
> Attached is a patch to apply on src/formats.c. With it
> cell_format_classify() now uses regexp (in fact it's in
> cell_format_is_number()).
>
> Some stupid questions :
>
>  - where can I put regcomp so it is called only once ?
You should be able to keep them as static variables initialized in
    currency_date_format_init

>  - I've define a new function find_currency(), should I declare it in
> formats.h ?
It does not look like it is globally useful.  Please make it static.

Nice work this is much better than the existing approach.  However,
a few tweaks.

1) please watch your formatting
int
foo ()
{

in place of
int foo () {

2) As you mention it would be better to move the compiled regexps
  into the init routines.  This can be time sensitive when classifying
  values as dates or numbers.

3) the curreny formats are not quite ready.  you need to check for
    - currency placement (see currency_date_format_init)
    - and optional single spaces between currency and value.
  
4) The code in src/dialogs/dialog-cell-format.c:generate_format
eg :
   Is not as advanced about the currency placement or spacing as it
   needs to be.  We can look up that info for the current locale,
   but we don't have a locale -> currency map nor can we assume that
   that all of the locales are available.

   Plan to merge the generate format code into src/formats.c so that
   there is enough info FormatCharacteristics to handle the
   generation.

> As it is my first patch on Gnumeric (and the first time I use regexp in
> a program) please tell me if anything is bad in my code.
Excellent start.

Thanks
    Jody