[armedbear-devel] patch for converting LispErrors to ProgramErrors in Closure.java, also removes some of the assertions

[Ville Voutilainen]

It looks like some of the assertions may be actually valid as
assertions. The remaining ones are:

1) in Closure constructor, parsing of parameters

            else if (obj instanceof Cons)
              {
                if (state == STATE_AUX)
                  {
                    Symbol sym = checkSymbol(obj.car());
                    LispObject initForm = obj.cadr();
                    Debug.assertTrue(initForm != null);
                    if (aux == null)
                      aux = new ArrayList<Parameter>();
                    aux.add(new Parameter(sym, initForm, AUX));
                  }

I don't see how to create a null initform other than creating a Cons
that has a null car or cdr. And
I don't know how to do that from lisp code. To me, that looks like it
requires serious abuse from the
Java side to accomplish. And I don't think that would be the only
place where the validity of a Cons
would need to be checked. I'm inclined to just remove that assertion completely.

2) again in Closure constructor,

    else
      {
        // Lambda list is empty.
        Debug.assertTrue(lambdaList == NIL);
        arity = 0;
        maxArgs = 0;
      }

That's an else branch of if (lambdaList instanceof Cons), and it looks
like it asserts that our own
code in Closure is correct. I'm inclined to keep that assertion as is.

3)

    if (arity >= 0)
      Debug.assertTrue(arity == minArgs);

a couple of lines down, check that minArgs is equal to arity if arity
isn't negative. Looks correct to
me, I'm inclined to keep it as is.

4)     if (optionalParameters.length == 0 && keywordParameters.length == 0)
      args = fastProcessArgs(args);
    else
      args = processArgs(args, thread);
    Debug.assertTrue(args.length == variables.length);

in the array version of execute. That looks like it verifies that our
argument processing works,
and looks correct to me. I'm inclined to keep it as is.

5) Parameter.processInitForm:

     if (initForm.constantp())
        {
          if (initForm instanceof Symbol)
            return initForm.getSymbolValue();
          if (initForm instanceof Cons)
            {
              Debug.assertTrue(initForm.car() == Symbol.QUOTE);
              return initForm.cadr();
            }

I'm not sure what is going on here. I don't know how to concoct a
lambda that has a constant
initform for a parameter so that the initform is Cons but is not
quoted. Granted, I've mainly tested
these changes with defuns, so I wouldn't be surprised if such a
failure case can be demonstrated.
I have no inclination here, this looks like it should probably be a
ProgramError rather than an assert.


Please review these cases (and the patch itself) and comment. I ran
the tests and I have a failure
in MAKE-BROADCAST-STREAM.8 that my baseline doesn't have, but I'm not
sure if it's related
to these changes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: closure-asserts.patch
Type: text/x-patch
Size: 2602 bytes
Desc: not available
URL: <https://mailman.common-lisp.net/pipermail/armedbear-devel/attachments/20100104/ac7b1f48/attachment.bin>