Expanded span printing #29995
Expanded span printing #29995
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| cm.span_to_string(sp)); | ||
| assert_eq!(sstr, res_str); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: reinstate the newline at the end of the file
|
This looks good. I would perhaps try a recursive approach rather than iterative - since we expect traces to be fairly short and acyclic, and there is no pressure to be ultra-fast. |
| while cur_span.expn_id != NO_EXPANSION && cur_span.expn_id != COMMAND_LINE_EXPN { | ||
| let old_span = cur_span; | ||
| let new_span = self.with_expn_info(old_span.expn_id, | ||
| |ei| ei.map(|ei| ei.call_site)); |
There was a problem hiding this comment.
You expand iteratively down the call_site branch, but only one step down the callee branch (below), it seems to me you should be iterating to the end of both branches (which would be easier if you were being recursive). At that point you probably need to think about indentation or something to distinguish the structure.
DanielJCampbell
force-pushed
the
Expanded-Span-Printing
branch
from
e9010c4 to
13f059d
Compare
|
Refactored to recurse down callee and call-site spans |
| /// CALLEE: | ||
| /// Callee span, or 'None' | ||
| /// CALLSITE: | ||
| /// Callsite span, or 'None' |
There was a problem hiding this comment.
Formatting of the output:
- I would not use caps for
callee/callsite- no need to draw attention to the headings - I would truncate the snippet to the smaller of 50 chars/one line +
... - rather than printing
None, how about not printing anything at all (i.e., no heading or span)
|
I like this approach much better! Thanks for making the changes. I think all these comments are fairly minor. |
|
Committed the suggested changes. |
| |ei| ei.map(|ei| ei.call_site.clone())); | ||
|
|
||
| indent.push_str(" "); | ||
| let mut flag = true; |
There was a problem hiding this comment.
could you give flag a more descriptive name please?
| first line. | ||
| Callsite: | ||
| blork.rs:1:1: 1:12 | ||
| first line. |
There was a problem hiding this comment.
Could you wrap he snippets in back ticks please - it'll make it easier to distinguish the code snippets from the rest
|
Last two comments! r+ with those addressed. |
DanielJCampbell
force-pushed
the
Expanded-Span-Printing
branch
2 times, most recently
from
b0606e5 to
5ab470b
Compare
|
@bors: r+ |
|
📌 Commit 5ab470b has been approved by |
DanielJCampbell
force-pushed
the
Expanded-Span-Printing
branch
from
5ab470b to
6b6fb7b
Compare
|
@bors: r+ |
|
📌 Commit 6b6fb7b has been approved by |
r? @nrc