Skip to content

Add error description for E0174 - #33920

Merged
bors merged 1 commit into
rust-lang:masterfrom
cristianoliveira:error-E0174-explanation
Jun 6, 2016
Merged

Add error description for E0174#33920
bors merged 1 commit into
rust-lang:masterfrom
cristianoliveira:error-E0174-explanation

Conversation

@cristianoliveira

@cristianoliveiracristianoliveira commented May 28, 2016

Copy link
Copy Markdown
Contributor

Reference for issue: #32777

r? @GuillaumeGomez

Hey Guillaume, sorry for taking too long to do it. I got some unexpected work during the week.

Waiting for your review :)

@rust-highfive

rust-highfive commented May 28, 2016

Copy link
Copy Markdown
Contributor

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

GuillaumeGomez
GuillaumeGomez reviewed May 28, 2016
View reviewed changes
Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

@GuillaumeGomezGuillaumeGomezMay 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A bit badly formulated.

@cristianoliveiracristianoliveiraMay 28, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm going change to the original short description error: "explicit use of unboxed closure methods are experimental"

@GuillaumeGomezGuillaumeGomezMay 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A bit more explicit isn't a problem. It's just badly formulated like I said.

@GuillaumeGomez

GuillaumeGomez commented May 28, 2016

Copy link
Copy Markdown
Member

Hey Guillaume, sorry for taking too long to do it. I got some unexpected work during the week.

No problem! 😃

So there are some sentences that are badly formulated and a few nits. Nothing big. Thanks for your PR!

@cristianoliveira

cristianoliveira commented May 28, 2016

Copy link
Copy Markdown
ContributorAuthor

Done. :)

GuillaumeGomez
GuillaumeGomez reviewed May 28, 2016
View reviewed changes
Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

@GuillaumeGomezGuillaumeGomezMay 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just noticed: why this functions' lines have an extra blank space at the beggining?

@cristianoliveiracristianoliveiraMay 28, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ops... I didn't notice also

@GuillaumeGomez

GuillaumeGomez commented May 28, 2016

Copy link
Copy Markdown
Member

Seems good for me but I'd prefer to have a second opinion on this. Thanks for your work! :)

cc @steveklabnik

PS: don't forget to squash your commits.

@cristianoliveira

cristianoliveira commented May 28, 2016

Copy link
Copy Markdown
ContributorAuthor

For sure. Let's see what @steveklabnik says so I squash them.

steveklabnik
steveklabnik reviewed May 31, 2016
View reviewed changes
Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

@steveklabniksteveklabnikMay 31, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"that are an experimental feature in this version of Rust."

@cristianoliveiracristianoliveiraMay 31, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I got it!

@cristianoliveira
cristianoliveiraforce-pushed the error-E0174-explanation branch 2 times, most recently from 9c2f10c to 15fdbf0CompareMay 31, 2016 13:16
@cristianoliveira

cristianoliveira commented May 31, 2016

Copy link
Copy Markdown
ContributorAuthor

Done.

GuillaumeGomez
GuillaumeGomez reviewed Jun 4, 2016
View reviewed changes
Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

@GuillaumeGomezGuillaumeGomezJun 4, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

’ seems weird for me. ' wouldn't be better in here? cc @steveklabnik

@cristianoliveiracristianoliveiraJun 4, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It comes from the documentation take a look:
https://doc.rust-lang.org/book/closures.html#closure-implementation

@GuillaumeGomezGuillaumeGomezJun 4, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure it applies here.

@cristianoliveiracristianoliveiraJun 4, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I see... There is another case and they have used (').
https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/diagnostics.rs#L1548

Let me change it.

@GuillaumeGomez

GuillaumeGomez commented Jun 4, 2016

Copy link
Copy Markdown
Member

Just a few more nits and it'll be good to go!

@cristianoliveira
cristianoliveiraforce-pushed the error-E0174-explanation branch 2 times, most recently from 38dff9c to 4c3d653CompareJune 4, 2016 17:39
@GuillaumeGomez

GuillaumeGomez commented Jun 4, 2016

Copy link
Copy Markdown
Member

All good, thanks!

@bors: r+ rollup

@bors

bors commented Jun 4, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 4c3d653 has been approved by GuillaumeGomez

@eddybeddyb mentioned this pull request Jun 5, 2016
Merged
eddyb
eddyb reviewed Jun 6, 2016
View reviewed changes
Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

@eddybeddybJun 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing argument type to Fn.

@cristianoliveiracristianoliveiraJun 6, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Let me fix it.

@eddyb

eddyb commented Jun 6, 2016

Copy link
Copy Markdown
Contributor

@bors r-

@cristianoliveira
cristianoliveiraforce-pushed the error-E0174-explanation branch from 4c3d653 to 45e647dCompareJune 6, 2016 04:02
@cristianoliveira

cristianoliveira commented Jun 6, 2016

Copy link
Copy Markdown
ContributorAuthor

Fixed

@eddyb

eddyb commented Jun 6, 2016

Copy link
Copy Markdown
Contributor

@bors r=GuillaumeGomez

@bors

bors commented Jun 6, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 45e647d has been approved by GuillaumeGomez

@bors

bors commented Jun 6, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 45e647d with merge 4a4a13a...

bors added a commit that referenced this pull request Jun 6, 2016
@bors
Auto merge of#33920- cristianoliveira:error-E0174-explanation, r=Gu…
4a4a13a
…illaumeGomez
Add error description for E0174
Reference for issue: #32777
r? @GuillaumeGomez
Hey Guillaume, sorry for taking too long to do it. I got some unexpected work during the week.
Waiting for your review :)
@bors

bors commented Jun 6, 2016

Copy link
Copy Markdown
Collaborator

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors
bors merged commit 45e647d into rust-lang:masterJun 6, 2016
@cristianoliveira
cristianoliveira deleted the error-E0174-explanation branch June 6, 2016 12:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@GuillaumeGomezGuillaumeGomez

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cristianoliveira@rust-highfive@GuillaumeGomez@bors@eddyb@steveklabnik