Skip to content

Introduce BacktraceFormatter - #23

Closed
st0012 wants to merge 2 commits into
ruby:masterfrom
st0012:introduce-backtrace-formatter
Closed

Introduce BacktraceFormatter#23
st0012 wants to merge 2 commits into
ruby:masterfrom
st0012:introduce-backtrace-formatter

Conversation

@st0012

@st0012st0012 commented May 22, 2021

Copy link
Copy Markdown
Member

This class will help us format/colorize the backtrace more easily. These are the responsibilities of each class involved in backtrace display:

  • ThreadClient
    • Retrieve frames
    • Compute line prefix and combine it with the trace to become a full backtrace line
    • Print backtrace lines
  • FrameInfo
    • Store each frame's information
    • Compute components' string representation (currently, return_value_str, call_identifier_str, and location_str)
  • BacktraceFormatter

This change doesn't change the output

❯ exe/rdbg -e 'b 20;; c ;; bt ;; info ;; q!' -e c target.rb
[1, 10] in target.rb
=> 1| class Foo
2| def first_call
3| second_call(20)
4| end
5|
6| def second_call(num)
7| third_call_with_block do |ten|
8| forth_call(num, ten)
9| end
10| end
=>#0 <main> at target.rb:1
(rdbg:init) b 20
#1 line bp /Users/st0012/projects/debug/target.rb:20 (return)
(rdbg:init) c
[15, 23] in target.rb
15| yield(10)
16| end
17|
18| def forth_call(num1, num2)
19| num1 + num2
=> 20| end
21| end
22|
23| Foo.new.first_call
=>#0 Foo#forth_call(num1=20, num2=10) at target.rb:20 #=> 30
#1 block{|ten=10|} in second_call at target.rb:8
# and 4 frames (use `bt' command for all frames)
Stop by #1 line bp /Users/st0012/projects/debug/target.rb:20 (return)
(rdbg:init) bt
=>#0 Foo#forth_call(num1=20, num2=10) at target.rb:20 #=> 30
#1 block{|ten=10|} in second_call at target.rb:8
#2 Foo#third_call_with_block(block=#<Proc:0x00007fbc021380d8 target.rb:7>) at target.rb:15
#3 Foo#second_call(num=20) at target.rb:7
#4 first_call at target.rb:3
#5 <main> at target.rb:23
(rdbg:init) info
=>#0 Foo#forth_call(num1=20, num2=10) at target.rb:20 #=> 30
%self => #<Foo:0x00007fbc02138380>
%return => 30
num1 => 20
num2 => 10
@ivar1 => 10
@ivar2 => 20
(rdbg:init) q!

@ko1

ko1 commented May 22, 2021

Copy link
Copy Markdown
Collaborator

To me it seems like overkill to introduce a new class.

@ko1ko1 closed this May 22, 2021
@st0012

st0012 commented May 22, 2021

Copy link
Copy Markdown
MemberAuthor

@ko1 I see. But would you consider accepting the colorization I tested in this PR? st0012#1
I decided to have such class because when I testing different formats for #13 and colorization, I found that having such class makes the implementation a lot easier. Without it, I had to touch both FrameInfo and ThreadClient classes without a clear boundary.

@st0012

Copy link
Copy Markdown
MemberAuthor

Also, given the amount of frame information we have, it'd be better to have a formatter class extracted for customization like mentioned in #13 (comment).

Personally, I'm very interested in maintain this area (presenting frame information) and I've been doing similar work in my own project for more than a year.

@ko1

ko1 commented May 22, 2021

Copy link
Copy Markdown
Collaborator
  • Proc should be enough for custom fornatter
  • Coloring should be used by other features, so should not be in this specific class.

@st0012

Copy link
Copy Markdown
MemberAuthor

Proc should be enough for custom fornatter

I thought a class was necessary because some formats requires computation on all the frames, like the With Separator (Aligned) format proposed in #13. But since we probably won't use it as the default format, I agree that using a class seems redundant 👍

Coloring should be used by other features, so should not be in this specific class.

Yeah that seems more reasonable. Perhaps we can reuse the coloring logic implemented in irb? I've opened #24 as an example.

@st0012
st0012 deleted the introduce-backtrace-formatter branch May 24, 2021 03:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@st0012@ko1