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 the output of the column! macro 1 based #46977

Merged
merged 3 commits into from
Dec 27, 2017
Merged

Conversation

est31
Copy link
Member

@est31 est31 commented Dec 24, 2017

Fixes #46868.

I didn't add any regression tests as the change already had to change tests inside the codebase.

r? @dtolnay

@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 24, 2017
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 24, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

I agree that this change is sufficiently tested by the existing tests.

/// The expanded expression has type `u32` and is 1-based, so the first line
/// in each file evaluates to 1, the second to 2, etc. This is consistent
/// with error messages by common compilers or popular editors.
/// The returned column is not the invocation of the `line!` macro itself,
Copy link
Member

Choose a reason for hiding this comment

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

This should say the returned *line*.

Copy link
Member

Choose a reason for hiding this comment

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

Since we are touching this line anyway, I find "not the invocation of the `line!` macro itself" a bit misleading, because it is totally possible and expected for the return value to be the line of the invocation of the line macro itself when not being called from another macro. It may be clearer as "not necessarily the line of the `line!` invocation itself".

(Okay to ignore this if you want, since it was that way before.)

@est31
Copy link
Member Author

est31 commented Dec 25, 2017

I've fixed the pointed out mistake. General improvement of the docs is best done in a separate PR IMO.

re-r? @dtolnay

@dtolnay
Copy link
Member

dtolnay commented Dec 25, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Dec 25, 2017

📌 Commit 6081989 has been approved by dtolnay

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! I filed #46997 to follow up on the other sentence.

@bors
Copy link
Contributor

bors commented Dec 25, 2017

⌛ Testing commit 6081989 with merge cfdebb52f3018996c2d5cf9566019bb18607dc26...

@bors
Copy link
Contributor

bors commented Dec 25, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Dec 25, 2017

@bors retry #47002

@bors
Copy link
Contributor

bors commented Dec 26, 2017

⌛ Testing commit 6081989 with merge 870b94f02d866df47ec5335f08a1348e5703448e...

@bors
Copy link
Contributor

bors commented Dec 26, 2017

💔 Test failed - status-travis

@estebank
Copy link
Contributor

@bors retry

@bors
Copy link
Contributor

bors commented Dec 27, 2017

⌛ Testing commit 6081989 with merge 3fd27b2...

bors added a commit that referenced this pull request Dec 27, 2017
 Make the output of the column! macro 1 based

Fixes  #46868.

I didn't add any regression tests as the change already had to change tests inside the codebase.

r? @dtolnay
@bors
Copy link
Contributor

bors commented Dec 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: dtolnay
Pushing 3fd27b2 to master...

@bors bors merged commit 6081989 into rust-lang:master Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.

6 participants