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.
