Hi Kevin,
I basically agree with most of your remarks. Here are some I don't
quite agree with or that deserve discussion.
Kevin Ryde <[EMAIL PROTECTED]> writes:
>> +* i18n Introduction:: Introduction to Guile's i18n support.
>
> "i18n" is a horrible geek-ish abbreviation, much better written in
> full in the manual.
Agreed, but that's only the name of the Info node. Changing it here
would make the line too long for Info.
>> [EMAIL PROTECTED] {Scheme Variable} %global-locale
>
> I'm worried if that should be a variable, rather than a function.
I wondered too. My conclusion is that there's nothing to be afraid of:
after all, it's a special object and we have full control over it
(pretty much like the EOF object, for instance[*]).
>> +This function is based on the C library's @code{strtod} function
>
> Not any more? Or just in a broadly similar style?
It's based on `strtod', really---I think you're mixing this and
`number->locale-string'.
>> [EMAIL PROTECTED] {Scheme Procedure} locale-yes-regexp [locale]
>> [EMAIL PROTECTED] {Scheme Procedure} locale-no-regexp [locale]
>
> These aren't gnu extensions are they?
No, they're not:
http://www.opengroup.org/onlinepubs/009695399/basedefs/langinfo.h.html
>> +(define (locale-encoding . locale)
>> + (apply nl-langinfo CODESET locale))
>
> I think you should implement almost all these basic bits in C, and not
> have a big nl-langinfo. It'll be easier for a future localeconv
> conditionals, or some of the DOS apis. It'll also be smaller and
> faster to load and run.
>
>> +(define-vector-langinfo-mapping locale-day-short
>> + (ABDAY_1 ABDAY_2 ABDAY_3 ABDAY_4 ABDAY_5 ABDAY_6 ABDAY_7)
>> + ("Sun" "Mon" "Tue" "Wed" "Thu" "Fri" "Sat"))
>
> These in C too I think.
I'm not sure about it. Writing all those bits in C would have clear
short-term advantages, mainly faster loading. That said, beside the
fact that I'm a bit lazy and don't really feel like rewriting the thing
in C ATM ;-), I think the above Scheme constructs are easier to work
with (to maintain, to add `localeconv' support, etc.) than a C
equivalent. So I'd be tempted to keep them as is.
(Besides, I don't think it's very high priority, especially since it
doesn't have any impact on the API itself.)
>> number formatting ...
>>
>> + (let loop ((index (- strlen 1))
>> + (grouping grouping)
>> + (since-separator 0)
>> + (result ""))
>
> Consider breaking out of the loop when grouping is null, and in any
> case going group by group instead of character by character.
Yes, indeed!
> The result could be built as a list of strings to be concatenated at
> the end too (string-concatenate/reverse or whatever it is).
What does it buy us to apply `string-append' at the end rather than
`string-append' at each step?
>> ;;; Local Variables:
>> ;;; coding: latin-1
>
> The fallbacks are all ascii aren't they?
I prefer to explicitly specify the encoding of non-ASCII files.
Thanks for the detailed review!
Ludo'.
[*] Actually, the EOF object _is_ available in R6RS, but it is returned
by the `eof-object' procedure. The following post sheds some light
on this decision:
http://www.r6rs.org/r6rs-editors/2006-July/001536.html . I'm not
sure this reasoning would apply well here: We already have two
implementations of `%global-locale' and both can happily deal with
it.
_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel
