Steel Bank Common Lisp

wanted: better error reporting in ASSERT

Reported by Tobias C. Rittweiler on 2011-05-28
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
SBCL
Low
Unassigned

Bug Description

I wish (ASSERT (FOO X Y)) reported "Assertion (FOO <val1> <val2>) failed."

Changed in sbcl:
importance: Undecided → Low
status: New → Triaged
tags: added: easy feature
Alexandra Barchunova (abarch) wrote :

The patch implements the proposed behavior for ASSERT. Tests are included.

I originally used (CONSTANTP PLACE ENV) but i had to take ENV out because otherwise the build failed. Presumably it happened due to early use of ASSERT.

During tests of the patch, i encountered an error in float.pure.lisp in the expression (assert (typep (nth-value 1 (ignore-errors (float-radix "notfloat"))) 'type-error)): the subexpression (nth-value ...) seems to return NIL. My macro expansion is here: http://paste.lisp.org/display/136940 . Am I doing something wrong? or is it a bug in IGNORE-ERRORS?

tags: added: review

To answer the question regarding the new type failure: it's very unlikely to be a bug in IGNORE-ERRORS. Does the evaluation of subforms in your evaluator handle multiple values correctly? (Another possibility that I seem to remember seeing fly past on IRC somewhere was that the error handling in FLOAT-RADIX is not quite right, but that is based on possibly faulty memory).

Could you update the patch to HEAD? There'll be a conflict in the change to NEWS, I'm afraid. Thanks.

Right, there's a slight bug in FLOAT-RADIX.

I think we should try and avoid duplicating temporary argument forms in the expansion. Currently, forms are copied for the initial and the step form in DO, and that's not good for compile times… especially when ASSERTs are nested (e.g. via inlining).

That'll probably be most easily achieved with TAGBODY.

Re CONSTANTP, you probably need to use the cross-compiler's version (sb!xc:constantp).

Alexandra Barchunova (abarch) wrote :

I will try to integrate the feedback in the new version tonight.

Alexandra Barchunova (abarch) wrote :

I revised the patch according to your feedback, however now there is new test failure:

Finished running tests.
Status:
 Expected failure: interface.pure.lisp / (SLEEP NON-CONSING)
 Expected failure: debug.impure.lisp / BACKTRACE-INTERRUPTED-CONDITION-WAIT
 Expected failure: dynamic-extent.impure.lisp / (NO-CONSING SPECIALIZED-DX-VECTORS)
 Failure: dynamic-extent.impure.lisp / (NO-CONSING DX-RAW-INSTANCES)
 Expected failure: packages.impure.lisp / USE-PACKAGE-CONFLICT-SET
 Expected failure: packages.impure.lisp / IMPORT-SINGLE-CONFLICT
 Expected failure: walk.impure.lisp / (WALK-LET* HAIRY-SPECIALS)
 Expected failure: walk.impure.lisp / (WALK-LET* HAIRY-SPECIALS)

I don't know whether my changes have caused the test failure.

Paul Khuong (pvk) wrote :

The test failure is likely caused by the use of assignment: the compiler
does a much better job on purely-functional code, especially for unboxed
values. I'm not sure why intermediate values are computed with a LET
binding around the loop and SETF. Moving the LET bindings inside the
loop should fix that unexpected consing.

Alexandra Barchunova (abarch) wrote :

thanks for the feedback! your suggestion is integrated now, and the tests run. updated patch is attached.

Paul Khuong (pvk) wrote :

Committed in f7808fb.

Changed in sbcl:
status: Triaged → Fix Committed
Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers