Re: [PATCH] Improved source properties and errors; => within case

[Mark H Weaver]

Hi Andy, thanks for the quick review!

Andy Wingo <address@hidden> writes:
> Patch set looks good to me.  Please push.

Great, thanks!  Of course I'll fix the following issues first.

> On Wed 08 Feb 2012 10:09, Mark H Weaver <address@hidden> writes:
>
>>  The way that source properties are stored means that Guile can only
>> -associate source properties with parenthesized expressions, and not, for
>> -example, with individual symbols, numbers or strings.  The difference
>> -can be seen by typing @code{(xxx)} and @code{xxx} at the Guile prompt
>> -(where the variable @code{xxx} has not been defined):
>> +associate source properties with parenthesized expressions and non-empty
>> +strings, and not, for example, with individual symbols or numbers.  The
>> +difference can be seen by typing @code{(xxx)} and @code{xxx} at the
>> +Guile prompt (where the variable @code{xxx} has not been defined):
>
> This isn't quite right; #*101010101 should probably get source info, no?

Yes, and indeed this patch _does_ add source info for bitvectors, but
I forgot to mention that in the doc.  I'll fix it.

> And is it useful to have an exception for empty strings?  I would think
> that it would be fine to return fresh empty strings.  The compiler would
> DTRT.  I don't care much though.

Currently 'read' returns the shared global 'scm_nullstr' for empty
strings.  We could remove that optimization though.  Maybe we should.
What do you think?

> Perhaps: "Everything but numbers, symbols, characters, and booleans get
> source information."  Dunno.

and keywords, and maybe some other things we're forgetting.  Good idea.
Another option is to explain it in terms of the core problem: only types
for which 'read' reliably returns a fresh object can have source
properties.  I'll think on this some more.

>> +    (syntax-case whole-expr ()
>> +      ((_ clause clauses ...)
>> +       #`(begin
>
> (This is in `cond').  Why is the begin needed here?

It's needed because the 'loop' returns a _list_ of expressions (of
length zero or one), to enable more graceful handling of the base case.
The outer 'loop' is guaranteed to return a list of length one, so I need
to either take the 'car' or wrap it in a 'begin'.

>> +                            #`((let ((t test))
>> +                                 (if t t #,@tail)))))
>
> Use `or' here.

I can't, because if it's the last clause, 'tail' will be '(), which
would generate (or test) which would be incorrect.  (or test) would
return #f is 'test' is false, but we actually want to return
*unspecified* in that case.

>> +    (syntax-case whole-expr ()
>> +      ((_ expr clause clauses ...)
>> +       (with-syntax ((key #'key))
>> +         #`(let ((key expr))
>> +             #,@(fold
>
> (In `case'.)  Likewise here, it would be good to avoid this use of an
> implicit `begin', of possible.

Hmm.  I don't know if this is what you meant, but it occurs to me that
as I've currently implemented them, both (cond (else (define x 5) x))
and (case 1 (else (define x 5) x)) are allowed.  I'll have to make sure
that those raise errors.  I guess that means I'll have to insert a '#f'
like I did for local-eval.  Do you see a better way?

    Thanks!
      Mark