Uh oh!
There was an error while loading. Please reload this page.
Less unused argument spew - #37729
Conversation
rust-highfive
commented
Nov 12, 2016
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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. |
There was a problem hiding this comment.
The remaining three cases in this diff all ended up bigger! Can they be special cased back to the old way?
brson
commented
Nov 15, 2016
Looks good. Can you add a test case with a multi-line span so we can see what it looks like? |
fb8da44 to
d8dd579CompareUnused arguments are attached to the main error as labels. This drops the traditional named/positional message split (except in the case of a *single* unused argument) for "unused" in interest of terseness. Closesrust-lang#37718.
d8dd579 to
6f0ba7eCompareDanielKeep
commented
Nov 18, 2016
I reverted the change to the single missing argument case, so it's back to the way it was. I also added a multi-line macro invocation to show what that looks like. The Travis failure appears to be unrelated to these changes: something about a lock file while it was setting up for the build. |
estebank
commented
Dec 5, 2016
@DanielKeep FYI, I believe BTW, this looks great! |
steveklabnik
commented
Jan 3, 2017
I've re-queued Travis. @brson are you happy with this PR now, assuming it passes? |
alexcrichton
commented
Feb 4, 2017
Sorry for the delay here @DanielKeep! It looks like the UI test may be failing on Travis though? If you want to rebase, fix tests, and ping me I'll r+ |
mrhota
commented
Feb 23, 2017
@DanielKeep ping? |
brson
commented
Feb 24, 2017
@bors r+ |
bors
commented
Feb 24, 2017
📌 Commit 6f0ba7e has been approved by |
bors
commented
Feb 25, 2017
⌛ Testing commit 6f0ba7e with merge abed1ac... |
eddyb
commented
Feb 25, 2017
@brson see @alexcrichton's message, the UI test just doesn't pass. @bors r- |
eddyb
commented
Feb 25, 2017
@bors force |
bors
commented
Feb 25, 2017
💔 Test failed - status-travis |
alexcrichton
commented
Feb 28, 2017
@bors: r=brson |
bors
commented
Feb 28, 2017
📌 Commit 6f0ba7e has been approved by |
alexcrichton
commented
Feb 28, 2017
@bors: retry |
frewsxcv
commented
Feb 28, 2017
There's a test failure here https://travis-ci.org/rust-lang/rust/jobs/176910049 |
frewsxcv
commented
Feb 28, 2017
@bors r- |
estebank
commented
Mar 13, 2017
@DanielKeep@frewsxcv It looks like the test stderr output has to be updated to latest master, that's all. |
alexcrichton
commented
Apr 1, 2017
Closing due to inactivity, but feel free to resubmit with tests fixed! |
Unused arguments are attached to the main error as labels. This drops
the traditional named/positional message split (except in the case of a
single unused argument) for "unused" in interest of terseness.
Closes#37718.
I tried using
MultiSpanwithstruct_err_span, but none of the spans were emitted. This was also the case with trying to attach aMultiSpanto theDiagnosticas a note.span_labelwas the only seemingly relevant method I found that would actually display.It feels a bit ugly, but works.
I also chose to merge the two branches for how to display the error for the sake of simplifying and unifying the diagnostic construction a little.