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

Make panic! error message GCC compatible #25152

Closed
wants to merge 2 commits into from

Conversation

krig
Copy link

@krig krig commented May 6, 2015

Reorders the output of thread error messages so that
the file:line location appears first on the line.

For example, this goes from

    thread 'test_fails' panicked at 'assertion failed: 0 == 1', src/main.rs:34

to

    src/main.rs:34: thread 'test_fails' panicked at 'assertion failed: 0 == 1'

Fixes: #25151

krig added 2 commits May 6, 2015 17:27
Reorders the output of thread error messages so that
the file:line location appears first on the line.

For example, this goes from

        thread 'test_fails' panicked at 'assertion failed: 0 == 1', src/main.rs:34

to

        src/main.rs:34: thread 'test_fails' panicked at 'assertion failed: 0 == 1'

Fixes: rust-lang#25151
In one case, the error output appended an extra newline character.
@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 @aturon (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. 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 CONTRIBUTING.md for more information.

@alexcrichton
Copy link
Member

From a high level perspective I would prefer to see the error message before I see where the error message is coming from, but integrating with existing tooling is indeed nice. I do think, however, that formal error messages shouldn't really be communicated through panic! but rather with a more principled API and/or output stream.

Another possibility would be to use the infrastructure in rust-lang/rfcs#1100 to have the test harness override this sort of behavior instead of having it apply to all Rust programs by default.

@krig
Copy link
Author

krig commented May 7, 2015

From a high level perspective I would prefer to see the error message before I see where the error message is coming from, but integrating with existing tooling is indeed nice.

I'm surprised, for me it's the complete opposite. And given that the regular compiler error messages start with file:line:, I don't think I'm alone in that.

I encounter this when running tests which call assert!, which in turn seems to call panic!. So the use case is for running cargo test as my build step in my IDE, and having the IDE make the test failures clickable, taking me to the failing test.

I'm not sure why you wouldn't want that to work by default...

@krig
Copy link
Author

krig commented May 18, 2015

I submitted a patch for rust-mode.el which makes this unnecessary in the specific case of emacs (rust-lang/rust-mode#63).

@alexcrichton
Copy link
Member

Ah sorry I thought I had already commented on this again! Looks like it feel under the radar...

And given that the regular compiler error messages start with file:line:, I don't think I'm alone in that.

I believe it depends on what application/framework/language you're using, for example C asserts for me locally mirror what Rust does today.

I'm not sure why you wouldn't want that to work by default...

I would love for this sort of interaction to work! I think, however, that tailoring this aspect for all usage of panic for one use case in an IDE, however, may not be the best motivation for the change. For example a targeted way to modify panic messages for just the test runner would work well here.

For now though it sounds like your local problems may in general be fixed, and otherwise I'm going to close this in favor of rust-lang/rfcs#1100 as I suspect it may be the route we wish to take for the test harness itself.

@krig
Copy link
Author

krig commented May 18, 2015

I would love for this sort of interaction to work! I think, however, that tailoring this aspect for all usage of panic for one use case in an IDE, however, may not be the best motivation for the change. For example a targeted way to modify panic messages for just the test runner would work well here.

Agreed, having a way to have more control over what panic outputs sounds great to me. Maybe the best solution is in general improvements to the assert! family of macros.

Thanks!

Sign up for free to 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.

Make panic! error output compatible with GCC error message
4 participants