Hi Andy, thanks for the quick review!
Andy Wingo <wi...@pobox.com> 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 <m...@netris.org> 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
