[armedbear-devel] patch for converting LispErrors to ProgramErrors in Closure.java, also removes some of the assertions
ville.voutilainen at gmail.com
Mon Jan 4 19:26:23 UTC 2010
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,
// 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.
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);
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.
if (initForm instanceof Symbol)
if (initForm instanceof Cons)
Debug.assertTrue(initForm.car() == Symbol.QUOTE);
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...
Size: 2602 bytes
Desc: not available
More information about the armedbear-devel