Re: [Kawa-commonlisp-dev] [PATCH] DECLARE processing.

[Charles Turner]

On 15 August 2012 08:40, Per Bothner <address@hidden> wrote:
> If there is a syntax error, just make sure SourceMessages#error
> is called - preferably by calling one of the Compilation#error
> methods is call.  Then  try to recover is a sane way - at least
> so we don't get an exception, and preferable to minimize the
> chance of follow-on useless error messages.  For example:
>
>   Object declNext =  declIterator.next();
>   if (! (declNext instanceof Pair)) {
>     Object save = tr.pushPositionOf(???);
>     tr.error("malformed declare");
>     tr.popPositionOf(save(;
>     break;
>   }
>
> The pushPositionOf/popPositionOf pair is optional, to provide a
> more precise error message.

Right, thanks. For the time being, It doesn't seem like I can do
perfect syntax checking for declare. For instance, this is obviously
not right:

(declare 'badger)

But until we have a method like isValidCLClassName, I don't know how
best to distinguish things like (quote badger) from (list badger),
etc. It doesn't cause any problems AFAICT, the type lookup just fails.
So the only error check for now is things like (declare 10) =>
malformed declare.

> You'll notice that I don't actually use SeqPosition/getIterator
> much, and certainly not in scan/rewrite of syntax forms.
> Not sure it buys you anything in this situation.

I've assumed you don't want this in the patch, so I've rewritten it to
the more common heavy-duty type casting and hard-to-follow car/cdring.
It's my opinion that the use of an iterator is both easier to
understand and a more common Java idiom, I didn't think it looked much
less efficient either. Anyway, I've removed the use of the iterators
now. I would like to explain what I don't like about your normal
rewriter layout, it's likely I've misunderstood something:

There seemed to be three common idioms using the non-iterator approach:

1) Make everything an Object in places where you'd rather use a Pair,
so that you can conveniently ask while (something != LList.Empty)
[note: something cannot be pair], I find this really annoying because
it means loads of ugly typecasting when you want to use
getCar()/getCdr().

2) Use Translator.listLength to find out in advance how many cdr's to
take. Means you can use Pair's, which reduces manual type-casting, but
you've got to traverse the list at least twice to do the processing...

3) Add an extra variable and use an unbounded for-loop:

Pair o_pair = ...;
Object o = o_pair;
for ( ; ; o = o_pair.getCdr()) {
  if (! (o instanceof Pair))
    break;
   ,,,
}

This gets really hard to understand what's going on, just take a look
at Lambda#rewriteFormals.

I've gone for option 1, as error checking is more straightforward with
it that the other two methods (it's easier still with iterators),
Unless I've missed better ways of doing this, it strikes me as wrong
that what is basically a list traversal is so hard to read/think
about.

Charles.

declare.patch
Description: Binary data