kawa-commonlisp-dev
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Per Bothner
Subject: Re: [Kawa-commonlisp-dev] [PATCH] DECLARE processing.
Date: Thu, 16 Aug 2012 17:48:44 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0 
On 08/15/2012 07:38 AM, Charles Turner wrote:

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.


One problem is that a list is not always a sequence,
and the iterator APIs are designed for sequences.
Consider for example:
  (declare (type integer . x))
or:
  (declare . y)
These improper lists need to be handled with a sane error message.
I think your code will throw a ClassCastException in some of these
cases.

Checking these cases seems more awkward and non-robust when using
iterators.


 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;
    ,,,
}


Worse:

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


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


I don't think it's that bad.  It's verbose, but at least it's obvious
what is going on.  Better naming conventions would probably help.


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.


Using pattern matching would probably be a helpful.
However, declare is complicated because it's not a macro in the
normal sense - it's a magic keyword for when parsing a <body>.
--
        --Per Bothner
address@hidden   http://per.bothner.com/
reply via email to
[Prev in Thread] Current Thread [Next in Thread]