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

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" <spk121@yahoo.com> 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'.

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

[Mike Gran]

On Mon, 2009-04-27 at 10:16 +0200, Ludovic Courtès wrote:
> Hello,

> Is `read_token2 ()' temporary?  (I didn't see it mentioned in the log.)
> 
> Otherwise I'd vote for a more descriptive name.
> 

It was meant to be temporary.

> >  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').
> 

OK

> > -    (pass-if "\\x08 backspace"
> > +    (pass-if "\\x08; backspace"
> >        (let ((port (open-output-string)))
> > -	(display "\x08" port)
> > +	(display "\x08;" port)

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

I'm getting there.  I pulled this idea of hex escapes out of the R6RS,
but, as a standard, I think it is a poor design.  Part of the reason for
moving hex escapes into their own function in the last push was to try
to centralize them, so that the reader option was possible.  

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

I can roll it back if you like.  I only changed it because of this line
in the Gnu coding standards.

"Please do not use the term 'illegal' to refer to erroneous input to a
computer program.  Please use 'invalid' for this, and reserve the term
'illegal' for activities prohibited by law."

> > --- 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?

If that line ends up remaining more permanent, I will.  

-Mike Gran