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

Audit traits and (partially) trait objects sections of Reference #25308

Closed
wants to merge 2 commits into from

Conversation

nham
Copy link
Contributor

@nham nham commented May 11, 2015

cc #16676

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

dispatched using _virtual method tables_ ("vtables"). Whereas most calls to
trait methods are "early bound" (statically resolved) to specific
implementations at compile time, a call to a method on an trait objects is
only resolved to a vtable entry at compile time. The actual
Copy link
Member

Choose a reason for hiding this comment

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

This sentence threw me for a bit of a spin, I think the grammar around "on an trait objects is" may need to be updated slightly, but it was a little odd talking about late bindings, then early bindings, then back to late bindings. Perhaps this could be restructured a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made an attempt at improving this.

@alexcrichton
Copy link
Member

Thanks for the edits! I think there's an interesting balance in the reference manual between defining what you can do as well as being a bit tutorial-like (because the book I believe also covers much of this material), but I think the updates here are fine regardless.

@steveklabnik
Copy link
Member

Same.

@bors
Copy link
Contributor

bors commented May 13, 2015

☔ The latest upstream changes (presumably #25340) made this pull request unmergeable. Please resolve the merge conflicts.

@steveklabnik
Copy link
Member

@nham this still can't merge, would you mind rebasing?

@nham
Copy link
Contributor Author

nham commented May 13, 2015

@steveklabnik On it.

The issue I'm seeing now is that @nikomatsakis made big changes to the "Traits" section as well, and in looking over his changes I'm now confused about the terminology here. My reading of Niko's changes is that:

  • a trait consists entirely of some collection of associated items (functions/types/constants/blah)
  • associated functions that take a self parameter are called "methods"
  • associated functions that do not take a self parameter are called "static methods"

This doesn't seem to be consistent with @alexcrichton's suggestion that "associated function" is the new name for a static method.

@nham
Copy link
Contributor Author

nham commented May 13, 2015

@steveklabnik I've rebased now.

@nikomatsakis
Copy link
Contributor

I don't particularly like the term "static method" -- if I used it, it was unintentional. I think associated fns are better, with methods being the term for a fn that takes a self argument.

@alexcrichton
Copy link
Member

It looks like there's not any current mentions of a "static method", so this all looks good to me, thanks @nham!

@bors: r+ b2f486f

@alexcrichton
Copy link
Member

@bors: rollup

@bors
Copy link
Contributor

bors commented May 13, 2015

⌛ Testing commit b2f486f with merge 0835189...

bors added a commit that referenced this pull request May 13, 2015
@bors
Copy link
Contributor

bors commented May 13, 2015

💔 Test failed - auto-mac-64-opt

@nham
Copy link
Contributor Author

nham commented May 13, 2015

Sorry, this was failing a test due to one of the code blocks. I think I've fixed it.

@steveklabnik
Copy link
Member

this needs 87c903a to fix it

@steveklabnik
Copy link
Member

(i've added it to the rollup)

bors added a commit that referenced this pull request May 13, 2015
@nham
Copy link
Contributor Author

nham commented May 13, 2015

Oops, I shouldn't have squashed. Sorry, and thanks.

@alexcrichton
Copy link
Member

@bors: r+ 6ca22f8

No worries!

@bors
Copy link
Contributor

bors commented May 13, 2015

☔ The latest upstream changes (presumably #25384) made this pull request unmergeable. Please resolve the merge conflicts.

@nham
Copy link
Contributor Author

nham commented May 13, 2015

Looks like this was merged in #25384 . Closing.

@nham nham closed this May 13, 2015
Manishearth added a commit to Manishearth/rust that referenced this pull request May 14, 2015
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.

6 participants