[armedbear-devel] [REVISED] Patch for including relevant java stack frames

[Mark Evenson]

On 7/30/09 12:41 PM, Tobias C. Rittweiler wrote:
[…]
>> Comments please on whether this functionality would be useful, and
>> potential improvements.
>
> a) Why did you change SYS:BACKTRACE-AS-LIST to listify the frames? I
>     deliberately changed it to return a list of StackFrame objects, so
>     higher levels can decide whether they want to print java frames, or
>     not. (Which addresses your concern about "abstraction barrier".)

 From what I recall this was to support the use of the :frame and 
:inspect methods in the top-level REPL as it worked before.  One could 
select a frame in the backtrace via ":frame n" (where n was the frame 
you were interested in), and then issue an ":inspect *" to start walking 
through the frame.  This should certainly be kept for the LispStackFrame 
but maybe not for the JavaStackFrame.  I think I ran into one problem 
that the "~S" representation of these objects was pretty opaque (i.e. 
just "#<JAVA-STACK-FRAME>" and "#<LISP-STACK-FRAME"), but I am not quite 
sure of my reasoning right now.  I'll revisit this in course of the 
renamings you suggest below.

>     Perhaps because the name BACKTRACE-AS-LIST could be understand to do
>     implicit listification? I could follow that line. In fact, I'd have
>     liked to perform the following renamings, but opted out to change too
>     many things with my patch:
>
>        Rename BACKTRACE to PRINT-BACKTRACE
>
>        Define BACKTRACE to return a list of StackFrames.
>
>        Define BACKTRACE-AS-LIST to return a list of listified stack
>        frames.

That sounds quite reasonable.  Sure, it is a lot of renaming, but other 
than SLIME, I think our only customer is 'top-level.lisp', so as long as 
we preserve the behavior there we should be fine.  I completely agree 
that calling our main backtrace function BACKTRACE-AS-LIST is wrong.


> b) I don't like that
>
>        (frame-to-list #<JAVA-STACK-FRAME>) =>  ("class.meth(file.java:NN)")
>
>     I think it should return ("class.meth" :file "file.java" :line NN) so
>     higher levels can use that information. (For example `v' in SLDB.)

Agreed.  I had a version doing plist like things as well, which makes a 
lot more sense for tools further down the line.  I was going to make the 
whole result a plist so as not to trip up future reorderings.  Would you 
just have the first element of the list be a string?

> c) PRINT-FRAME contains an IGNORE-ERRORS which purpose I don't
>     understand. Mind you, you actually copied that from my patch, so it's
>     all my fault---I just can't remember why I put it in there. Do you
>     know? If so, please add a comment.

Nope, I thought *you* had the deep reason here, so I was just slaving 
away via cut and paste.  I'll remove it in the next iteration.


-- 
"A screaming comes across the sky.  It has happened before, but there
is nothing to compare to it now."