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

Use diagnostics for trace_macro instead of println #41520

Merged
merged 2 commits into from
May 9, 2017

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Apr 24, 2017

When using trace_macro, use span_labels instead of println:

note: trace_macro
  --> $DIR/trace-macro.rs:14:5
   |
14 |     println!("Hello, World!");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: expands to `println! { "Hello, World!" }`
   = note: expands to `print! { concat ! ( "Hello, World!" , "\n" ) }`

Fix #22597.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank
Copy link
Contributor Author

CC @brson @jonathandturner

@nikomatsakis
Copy link
Contributor

Do we have any tests of this feature? Maybe make a src/test/ui test?

@estebank estebank force-pushed the trace-macro branch 2 times, most recently from 708d494 to 929240d Compare April 25, 2017 01:47
@durka
Copy link
Contributor

durka commented Apr 25, 2017

That's... a ton of extra noise, compared to:

println! { "" }
print! { concat ! ( "" , "\n" ) }

especially if a macro has multiple expansion steps. What's the concrete benefit to users of the feature, here? Can you at least get rid of the "macro in external crate" bit?

@nikomatsakis
Copy link
Contributor

@durka line numbers seem like a win, but if they're just repeated N times, perhaps less useful that way. We could presumably make these attached notes without spans, right? (I've never actually used this utility...)

@durka
Copy link
Contributor

durka commented Apr 25, 2017

Maybe it's possible to attach all the expansion steps to one span? Like this:

note: trace_macro
  --> ../../src/test/run-pass/log_syntax-trace_macros-macro-locations.rs:27:5
   |
27 |     println!("");
   |     ^^^^^^^^^^^^^ expands to `println! { "" }`
   |     ^^^^^^^^^^^^^ expands to `print! { concat ! ( "" , "\n" ) }`

That would in fact be better than the current output, where you have to wade through all of it to find a particular macro's expansion. The rightward drift still bugs me a bit.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 25, 2017

@durka yes that was roughly what I had in mind, but probably more like

note: trace_macro
  --> ../../src/test/run-pass/log_syntax-trace_macros-macro-locations.rs:27:5
   |
27 |     println!("");
   |     ^^^^^^^^^^^^^ tracing macro expansion here
   |
   | note: expands to `println! { "" }`
   | note: expands to `print! { concat ! ( "" , "\n" ) }`

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2017
@arielb1
Copy link
Contributor

arielb1 commented May 2, 2017

@estebank - just a friendly ping to keep this on your radar

@estebank estebank force-pushed the trace-macro branch 2 times, most recently from c49cc0e to 0fabc21 Compare May 6, 2017 06:29
@estebank
Copy link
Contributor Author

estebank commented May 6, 2017

Updated.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 7, 2017
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 8, 2017

📌 Commit 8c9ad8d has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 9, 2017
Use diagnostics for trace_macro instead of println

When using `trace_macro`, use `span_label`s instead of `println`:

```rust
note: trace_macro
  --> $DIR/trace-macro.rs:14:5
   |
14 |     println!("Hello, World!");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: expands to `println! { "Hello, World!" }`
   = note: expands to `print! { concat ! ( "Hello, World!" , "\n" ) }`
```

Fix rust-lang#22597.
bors added a commit that referenced this pull request May 9, 2017
Rollup of 8 pull requests

- Successful merges: #41293, #41520, #41827, #41828, #41833, #41836, #41838, #41842
- Failed merges:
@bors bors merged commit 8c9ad8d into rust-lang:master May 9, 2017
@bors
Copy link
Contributor

bors commented May 9, 2017

⌛ Testing commit 8c9ad8d with merge bedd7da...

@bors
Copy link
Contributor

bors commented May 9, 2017

☔ The latest upstream changes (presumably #41846) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants