wanted: better error reporting in ASSERT

Bug #789497 reported by Tobias C. Rittweiler
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
SBCL
Fix Released
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
Revision history for this message
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
Revision history for this message
Christophe Rhodes (csr21-cantab) wrote :

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.

Revision history for this message
Paul Khuong (pvk) wrote : Re: [Bug 789497] Re: wanted: better error reporting in ASSERT

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).

Revision history for this message
Alexandra Barchunova (abarch) wrote :

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

Revision history for this message
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.

Revision history for this message
Alexandra Barchunova (abarch) wrote :

updated patch

Revision history for this message
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.

Revision history for this message
Alexandra Barchunova (abarch) wrote :

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

Revision history for this message
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  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.