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

improve "Doesn't live long enough" error #37174

Merged
merged 1 commit into from
Oct 22, 2016
Merged

Conversation

mikhail-m1
Copy link
Contributor

I've fixed only with same case

issue #36537 part of #35233
r? @jonathandturner

@sophiajt
Copy link
Contributor

This direction feels like an improvement, though looking back at #36537 I recall we'd decided that it would be something like:

struct_span_error(line 5 at '}, ....)
span_label(line 4 at 'a', "borrow occurs here")
span_label(line 5 at '}', "a dropped here while still borrowed")

It looks like you moved the span to the end of the block, which sounds right. I didn't see the labels for some of the errors you updated. You should add those, too, as I think they'll help the error messages be more readable.

You may also want to consider moving the tests you're updating to be UI tests. You can see examples in src/test/ui. They're pretty straightforward, the .stderr file just captures what the output is expected to look like.

@mikhail-m1
Copy link
Contributor Author

@jonathandturner Should I move all changed tests to ui?

@sophiajt
Copy link
Contributor

@mikhail-m1 - feel free to move all the tests you're updating with this PR to UI

@mikhail-m1 mikhail-m1 force-pushed the dnlle branch 2 times, most recently from 7b7a886 to 73422a9 Compare October 17, 2016 19:23
@mikhail-m1
Copy link
Contributor Author

@jonathandturner I created new directory in ui, is it ok?

@sophiajt
Copy link
Contributor

@mikhail-m1 - you can put them in src/test/ui/span

@mikhail-m1
Copy link
Contributor Author

mikhail-m1 commented Oct 17, 2016

@jonathandturner was moved

@sophiajt
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2016

📌 Commit 842b79d has been approved by jonathandturner

@mikhail-m1
Copy link
Contributor Author

two new tests in master branch

@sophiajt
Copy link
Contributor

@bors r-

Yeah, looks like there are a few that need to be updated:

    [compile-fail] compile-fail/dropck_tarena_cycle_checked.rs
    [compile-fail] compile-fail/dropck_tarena_unsound_drop.rs

@mikhail-m1
Copy link
Contributor Author

mikhail-m1 commented Oct 18, 2016

@jonathandturner I cannot find this files in master branch. All cfail tests passes.

edit: I've found in cfail-full

@mikhail-m1
Copy link
Contributor Author

@jonathandturner Should I put tests from compile-fail-fulldeps in ui too? There is no ui-fulldeps.

@sophiajt
Copy link
Contributor

@mikhail-m1 you can leave the fulldeps tests where they are. I'm not 100% sure they'll work as ui tests.

@mikhail-m1
Copy link
Contributor Author

test [ui] ui/dropck/dropck-eyepatch-extern-crate.rs ... FAILED
test [ui] ui/dropck/dropck-eyepatch-reorder.rs ... FAILED
test [ui] ui/dropck/dropck-eyepatch.rs ... FAILED

@mikhail-m1
Copy link
Contributor Author

@jonathandturner fixed

@sophiajt
Copy link
Contributor

@mikhail-m1 I think this is ready to land.

The wording for the temporary values can also be updated, but that can happen in a separate PR.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 21, 2016

📌 Commit e852775 has been approved by jonathandturner

@bors
Copy link
Contributor

bors commented Oct 21, 2016

⌛ Testing commit e852775 with merge a6fa572...

bors added a commit that referenced this pull request Oct 21, 2016
improve "Doesn't live long enough" error

I've fixed only with same case

issue #36537 part of  #35233
r? @jonathandturner
@bors bors merged commit e852775 into rust-lang:master Oct 22, 2016
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.

3 participants