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

Stabilize the panic_col feature #46762

Merged
merged 2 commits into from
Jan 10, 2018
Merged

Stabilize the panic_col feature #46762

merged 2 commits into from
Jan 10, 2018

Conversation

est31
Copy link
Member

@est31 est31 commented Dec 16, 2017

I've added the panic_col feature in PR #42938.
Now it's time to stabilize it!
Closes #42939.

@rust-highfive
Copy link
Collaborator

r? @TimNN

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

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 16, 2017
@TimNN
Copy link
Contributor

TimNN commented Dec 17, 2017

cc @rust-lang/libs: There seems nothing more to do here, does this require any kind of FCP?

@alexcrichton
Copy link
Member

@rfcbot fcp merge

Yeah for stabilizations we do indeed like to go through FCP!

@rfcbot
Copy link

rfcbot commented Dec 18, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 18, 2017
@alexcrichton alexcrichton assigned alexcrichton and unassigned TimNN Dec 18, 2017
@dtolnay
Copy link
Member

dtolnay commented Dec 18, 2017

Is it intentional that line numbers in a panic start at 1 but column numbers start at 0? I see that this is what line!() and column!() do as well. What are the advantages? Is it too late to change?

fn main() {
panic!("line 2 column 0");
}

@rfcbot concern 0

@sfackler
Copy link
Member

vim seems to start at 1 for lines and columns.

@est31
Copy link
Member Author

est31 commented Dec 18, 2017

Compiler errors in both Rust and gcc start at 1 for columns as well. If you have code like:

fn main() {
 panic!("line 2 column 0");
}

Rust will say the column is 1 and if you do kate src/main.rs:linenum:1 then it jumps to the space, not the panic.

I'd say this is a off-by-one error and a bug in the actual code that emits the panic column info. I'd also say that it's a bug with the column!() macro but what to do about that one might be something for a later discussion.

@est31
Copy link
Member Author

est31 commented Dec 18, 2017

Note that this code gives you the correct column number:

fn main() {
let v = [1,2,3];
v[7];
}

This is because this panic is directly emitted by the compiler and thus uses a separate way to obtain the column.

I guess I'll just add one to the column value for the panic macro case.

@est31
Copy link
Member Author

est31 commented Dec 19, 2017

Not sure how to add a regression test for this. Any ideas?

@sfackler
Copy link
Member

You should be able to make an rpass test that sets a panic handle that looks at the column.

@est31
Copy link
Member Author

est31 commented Dec 20, 2017

@sfackler good idea, will do

@dtolnay
Copy link
Member

dtolnay commented Dec 20, 2017

@rfcbot resolved 0

@dtolnay
Copy link
Member

dtolnay commented Dec 20, 2017

We discussed this with the libs team and we would be interested in fixing the column!() macro as well. I filed #46868 to follow up in a separate PR.

@est31 est31 changed the title Stabilize the panic_col feature [WIP] Stabilize the panic_col feature Dec 23, 2017
@est31
Copy link
Member Author

est31 commented Dec 23, 2017

I've marked this as WIP until the column issue is resolved.

@est31 est31 force-pushed the master branch 3 times, most recently from 0855471 to 131a9d8 Compare December 27, 2017 17:58
@est31 est31 changed the title [WIP] Stabilize the panic_col feature Stabilize the panic_col feature Dec 27, 2017
@est31
Copy link
Member Author

est31 commented Dec 27, 2017

#46977 is merged now!

I have added a test to verify that the returned column is 1 based.

@aturon @sfackler could you have a look and r+ the proposed FCP? Thanks!

@alexcrichton
Copy link
Member

alexcrichton commented Jan 4, 2018

ping @aturon @sfackler, y'all may be interested in this!

@sfackler
Copy link
Member

sfackler commented Jan 4, 2018

LGTM

@est31
Copy link
Member Author

est31 commented Jan 4, 2018

@sfackler can you do rfcbot reviewed? Or can someone check it on @sfackler 's behalf?

@carols10cents
Copy link
Member

ping @aturon this is waiting for your ticky box here!

@rfcbot
Copy link

rfcbot commented Jan 9, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 9, 2018
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 10, 2018

📌 Commit 131a9d8 has been approved by alexcrichton

@@ -329,7 +328,7 @@ impl<'a> Location<'a> {
///
/// panic!("Normal panic");
/// ```
#[unstable(feature = "panic_col", reason = "recently added", issue = "42939")]
#[stable(feature = "panic_col", since = "1.24")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be since = "1.25" now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

I've added the panic_col feature in PR rust-lang#42938.
Now it's time to stabilize it!
Closes rust-lang#42939.
@est31
Copy link
Member Author

est31 commented Jan 10, 2018

re-r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 10, 2018

📌 Commit 2491814 has been approved by alexcrichton

@frewsxcv
Copy link
Member

@bors rollup

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jan 10, 2018
Stabilize the panic_col feature

I've added the panic_col feature in PR rust-lang#42938.
Now it's time to stabilize it!
Closes rust-lang#42939.
bors added a commit that referenced this pull request Jan 10, 2018
Rollup of 5 pull requests

- Successful merges: #46762, #46777, #47262, #47285, #47301
- Failed merges:
@bors bors merged commit 2491814 into rust-lang:master Jan 10, 2018
@bors
Copy link
Contributor

bors commented Jan 10, 2018

☔ The latest upstream changes (presumably #47308) 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
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.