Re: [Guile-commits] GNU Guile branch, string-abstraction, updated. feda5bd1c08d69628ec9fdb5577d5c894b70b6e8

[Ludovic Courtès]

Hello,

A few notes regarding the latest changes in `string-abstraction'.

"Michael Gran" <address@hidden> writes:

> --- a/libguile/read.c
> +++ b/libguile/read.c
> @@ -186,13 +186,42 @@ static inline SCM scm_read_scsh_block_comment (int chr, 
> SCM port);
>  /* Read from PORT until a delimiter (e.g., a whitespace) is read.  Return
>     zero if the whole token fits in BUF, non-zero otherwise.  */
>  static inline int
> -read_token (SCM port, char *buf, size_t buf_size, size_t *read)
> +read_token (SCM port, scm_t_uint32 *buf, size_t buf_size, size_t *read)
>  {
>    *read = 0;
>  
>    while (*read < buf_size)
>      {
> -      int chr;
> +      scm_t_uint32 chr;
> +
> +      chr = scm_getc (port);
> +      chr = (SCM_CASE_INSENSITIVE_P ? CHAR_DOWNCASE (chr) : chr);
> +
> +      if (chr == (scm_t_uint32) EOF)
> +     return 0;
> +      else if (CHAR_IS_DELIMITER (chr))
> +     {
> +       scm_ungetc (chr, port);
> +       return 0;
> +     }
> +      else
> +     {
> +       *buf = chr;
> +       buf++, (*read)++;
> +     }
> +    }
> +
> +  return 1;
> +}
> +
> +static inline int
> +read_token2 (SCM port, char *buf, size_t buf_size, size_t *read)

Is `read_token2 ()' temporary?  (I didn't see it mentioned in the log.)

Otherwise I'd vote for a more descriptive name.

>  static scm_t_uint32

BTW, I'm wondering whether we should have a separate type for wide
chars, to make the intent clearer (say, `scm_t_char').

> --- a/test-suite/tests/ports.test
> +++ b/test-suite/tests/ports.test
> @@ -441,14 +441,14 @@
>       (display "x\a" port)
>       (= 1 (port-column port))))
>  
> -    (pass-if "\\x08 backspace"
> +    (pass-if "\\x08; backspace"
>        (let ((port (open-output-string)))
> -     (display "\x08" port)
> +     (display "\x08;" port)

[...]

>  (define exception:illegal-escape
> -  (cons 'read-error "illegal character in escape sequence: .*$"))
> +  (cons 'read-error "invalid character in escape sequence: .*$"))

[...]

>    (pass-if "CR recognized as a token delimiter"
>      ;; In 1.8.3, character 0x0d was not recognized as a delimiter.
> -    (equal? (read-string "one\x0dtwo") 'one))
> +    (equal? (read-string "one\x0d;two") 'one))

We're introducing subtle incompatibilities here, which we'd rather
avoid, by default at least.  Could you make that a reader option?

Likewise, I think we should avoid gratuitously changing exception
messages since, unfortunately, they are sort-of part of the API.

> --- a/test-suite/tests/regexp.test
> +++ b/test-suite/tests/regexp.test
> @@ -22,6 +22,8 @@
>    #:use-module (test-suite lib)
>    #:use-module (ice-9 regex))
>  
> +(setlocale LC_ALL "en_US.iso88591")

The user doesn't necessarily have this locale.  Can you instead use
something similar to `under-french-locale-or-unresolved' in `i18n.test'
or `find-latin1-locale' in `srfi-14.test', perhaps moving it to the
`(test-suite lib)' module?

Thanks,
Ludo'.