Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace_macro: Show both the macro call and its expansion. #42072. #42103

Merged
merged 1 commit into from
May 27, 2017

Conversation

jorendorff
Copy link
Contributor

See #42072 for the initial motivation behind this.

The change is not the minimal fix, but I want this behavior almost every time I use trace_macros.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (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.

@shepmaster
Copy link
Member

Thanks! We'll get @pnkfelix on the case soon!

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2017
@estebank
Copy link
Contributor

estebank commented May 19, 2017

Thanks for the PR!

For some time now notes accepts multiline strings. Given that there's some duplication, I'm wondering wether it'd be nice to have output along the lines of:

note: trace_macro
  --> $DIR/trace-macro.rs:14:5
   |
14 |     println!("Hello, World!");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: expanding this macro call to:
           `println! { "Hello, World!" }`
           `print ! ( concat ! ( "Hello, World!" , "/n" ) )`
           `$crate :: io :: _print ( format_args ! ( concat ! ( "Hello, World!" , "/n" ) )
           )`

@jorendorff
Copy link
Contributor Author

In the particular test in this patch, the TokenStreams mostly fit on one line each, but I suspect that's rare in practice.

Here's a noisier example:

note: trace_macro
 --> main.rs:5:5
  |
5 |     assert_eq!(vec![1, 2, 3,].len(), 3);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: expanding `assert_eq! { vec ! [ 1 , 2 , 3 , ] . len (  ) , 3 }`
  = note: to `{
          match ( & vec!(1 , 2 , 3 ,).len() , & 3 ) {
          ( left_val , right_val ) => {
          if ! ( * left_val == * right_val ) {
          panic ! (
          "assertion failed: `(left == right)` \
                                     (left: `{:?}`, right: `{:?}`)"
          , left_val , right_val ) } } } }`
  = note: expanding `panic! { "assertion failed: `(left == right)` \
                                     (left: `{:?}`, right: `{:?}`)"
          , left_val , right_val }`
  = note: to `{
          $crate :: rt :: begin_panic_fmt (
          & format_args ! (
          "assertion failed: `(left == right)` (left: `{:?}`, right: `{:?}`)" , left_val
          , right_val ) , {
          static _FILE_LINE : ( & 'static str , u32 ) = ( file ! (  ) , line ! (  ) ) ;
          & _FILE_LINE } ) }`

Note also that the second expanding: note here is not redundant with the immediately preceding to: note. Of course when it is redundant, we could certainly eliminate it.

@estebank
Copy link
Contributor

@bors r+

@jorendorff can you file a ticket to handle that case?

@bors
Copy link
Contributor

bors commented May 22, 2017

📌 Commit f8b66a0 has been approved by estebank

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 23, 2017
trace_macro: Show both the macro call and its expansion. rust-lang#42072.

See rust-lang#42072 for the initial motivation behind this.

The change is not the minimal fix, but I want this behavior almost every time I use `trace_macros`.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 23, 2017
trace_macro: Show both the macro call and its expansion. rust-lang#42072.

See rust-lang#42072 for the initial motivation behind this.

The change is not the minimal fix, but I want this behavior almost every time I use `trace_macros`.
@arielb1 arielb1 added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2017
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 26, 2017
trace_macro: Show both the macro call and its expansion. rust-lang#42072.

See rust-lang#42072 for the initial motivation behind this.

The change is not the minimal fix, but I want this behavior almost every time I use `trace_macros`.
@bors
Copy link
Contributor

bors commented May 27, 2017

⌛ Testing commit f8b66a0 with merge 5469410...

@bors
Copy link
Contributor

bors commented May 27, 2017

💔 Test failed - status-appveyor

@Mark-Simulacrum
Copy link
Member

@bors retry

  • appveyor timed out; not sure why though, nothing in log is immediately suspicious.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 27, 2017
trace_macro: Show both the macro call and its expansion. rust-lang#42072.

See rust-lang#42072 for the initial motivation behind this.

The change is not the minimal fix, but I want this behavior almost every time I use `trace_macros`.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 27, 2017
trace_macro: Show both the macro call and its expansion. rust-lang#42072.

See rust-lang#42072 for the initial motivation behind this.

The change is not the minimal fix, but I want this behavior almost every time I use `trace_macros`.
bors added a commit that referenced this pull request May 27, 2017
Rollup of 10 pull requests

- Successful merges: #42103, #42137, #42162, #42167, #42175, #42207, #42217, #42246, #42249, #42251
- Failed merges:
@bors
Copy link
Contributor

bors commented May 27, 2017

⌛ Testing commit f8b66a0 with merge 9337ad5...

bors added a commit that referenced this pull request May 27, 2017
trace_macro: Show both the macro call and its expansion. #42072.

See #42072 for the initial motivation behind this.

The change is not the minimal fix, but I want this behavior almost every time I use `trace_macros`.
@bors
Copy link
Contributor

bors commented May 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 9337ad5 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants