fix overflow due to multiline error span #31281

Merged
merged 4 commits into from Feb 2, 2016

Conversation

Projects
None yet
6 participants
@oli-obk
Contributor

oli-obk commented Jan 29, 2016

No description provided.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jan 29, 2016

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

Collaborator

rust-highfive commented Jan 29, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@mitaa

This comment has been minimized.

Show comment
Hide comment
@mitaa

mitaa Jan 29, 2016

Contributor

... oops.

Thanks for fixing this. (I guess adding yet another testcase, here for "end-highlighted-multiline-but-less-than-6-lines-span", would be overkill)

Contributor

mitaa commented Jan 29, 2016

... oops.

Thanks for fixing this. (I guess adding yet another testcase, here for "end-highlighted-multiline-but-less-than-6-lines-span", would be overkill)

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Jan 29, 2016

Contributor

@mitaa: the testcase would not help, we need to enable debug assertions in travis and homu. The test case for this situation was 10% of compile-fail ;)

Contributor

oli-obk commented Jan 29, 2016

@mitaa: the testcase would not help, we need to enable debug assertions in travis and homu. The test case for this situation was 10% of compile-fail ;)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Jan 29, 2016

Member

we need to enable debug assertions in travis and homu

In particular, I believe we have at least one buildbot that has --enable-debug turned one; but I also think it does not run the test suite. See in particular #27010 which suggested adding such a configuration, and rust-lang-deprecated/rust-buildbot#38 which turned on gating on it (but it does not run make check)

Member

pnkfelix commented Jan 29, 2016

we need to enable debug assertions in travis and homu

In particular, I believe we have at least one buildbot that has --enable-debug turned one; but I also think it does not run the test suite. See in particular #27010 which suggested adding such a configuration, and rust-lang-deprecated/rust-buildbot#38 which turned on gating on it (but it does not run make check)

mitaa and others added some commits Jan 29, 2016

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 29, 2016

Contributor

@oli-obk seems like it's still worth having focused tests, since many of us (e.g., me) run tests with --enable-debug-assertions

Contributor

nikomatsakis commented Jan 29, 2016

@oli-obk seems like it's still worth having focused tests, since many of us (e.g., me) run tests with --enable-debug-assertions

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 29, 2016

Contributor

But then there is a test? (Also, the travis failures look...bogus to me?)

Contributor

nikomatsakis commented Jan 29, 2016

But then there is a test? (Also, the travis failures look...bogus to me?)

@mitaa

This comment has been minimized.

Show comment
Hide comment
@mitaa

mitaa Jan 29, 2016

Contributor

But then there is a test? (Also, the travis failures look...bogus to me?)

Yes, but since the code path was never used, due to overflow, it is naturally off by one. (>=, instead of >)

Currently, running tests locally.. :-(

Contributor

mitaa commented Jan 29, 2016

But then there is a test? (Also, the travis failures look...bogus to me?)

Yes, but since the code path was never used, due to overflow, it is naturally off by one. (>=, instead of >)

Currently, running tests locally.. :-(

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 29, 2016

Contributor

@mitaa

Yes, but since the code path was never used, due to overflow, it is naturally off by one.

I don't understand what you mean by this.

Contributor

nikomatsakis commented Jan 29, 2016

@mitaa

Yes, but since the code path was never used, due to overflow, it is naturally off by one.

I don't understand what you mean by this.

@mitaa

This comment has been minimized.

Show comment
Hide comment
@mitaa

mitaa Jan 29, 2016

Contributor

@nikomatsakis

I don't understand what you mean by this.

Sorry, what I basically meant to say was that there is a test for this, that the travis failure is genuine, and that the final necessary fix is probably to change (hi.line - lo.line) > MAX_SP_LINES, which is off-by-one, to (hi.line - lo.line) >= MAX_SP_LINES

(make check-stage1-syntax passes locally for me with this fix)
(need to rerun make check due to probably bogus debuginfo test failures)

Contributor

mitaa commented Jan 29, 2016

@nikomatsakis

I don't understand what you mean by this.

Sorry, what I basically meant to say was that there is a test for this, that the travis failure is genuine, and that the final necessary fix is probably to change (hi.line - lo.line) > MAX_SP_LINES, which is off-by-one, to (hi.line - lo.line) >= MAX_SP_LINES

(make check-stage1-syntax passes locally for me with this fix)
(need to rerun make check due to probably bogus debuginfo test failures)

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Jan 30, 2016

Contributor

Travis likes it now

Contributor

oli-obk commented Jan 30, 2016

Travis likes it now

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 1, 2016

Contributor

@bors r+ rollup

Contributor

nikomatsakis commented Feb 1, 2016

@bors r+ rollup

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 1, 2016

Contributor

📌 Commit 54927ac has been approved by nikomatsakis

Contributor

bors commented Feb 1, 2016

📌 Commit 54927ac has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 1, 2016

Contributor

lgtm!

Contributor

nikomatsakis commented Feb 1, 2016

lgtm!

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Feb 1, 2016

bors added a commit that referenced this pull request Feb 2, 2016

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Feb 2, 2016

bors added a commit that referenced this pull request Feb 2, 2016

@bors bors merged commit 54927ac into rust-lang:master Feb 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@oli-obk oli-obk deleted the oli-obk:patch-2 branch Feb 2, 2016

@oli-obk oli-obk referenced this pull request in pnkfelix/collab-docs Apr 27, 2016

Closed

overflow in multiline error spans #3

@pnkfelix pnkfelix referenced this pull request in pnkfelix/collab-docs Jan 17, 2017

Closed

add dbaupp's noted overflow to the invalid span list #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment