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

[WIP] Start adding support for opt-in built-in traits #14371

Closed
wants to merge 13 commits into from

Conversation

flaper87
Copy link
Contributor

This is the first PR adding actual logic for this RFC.

It's still a work in progress but I'd like to start getting feedback from the community.

The PR consists in:

  • Adding a bounds checker visitor. As of now, this visitor 'only' checks trait implementations. When a trait implementation for a built-in trait is found, it iterates over the implementation type and checks whether it fulfills the trait.
  • Adding a function that gets the trait impl vtables for a type.
  • Implementing the Share trait for the types requiring it. Note that the implementations added are the ones required to make rust compile. There may definitely be missing implementations for this trait.
  • Implementing all the built-in traits for the built-in types

There are still some issues I'm working on. For instance, some fields in the typeck-bounds-share-impls are commented out because I hit a compiler bug if they're not. The compiler bug is not really clear to me but I'm working with Niko on figuring what's going on there.

(sorry for taking so long to post this)

cc @nikomatsakis @pcwalton

cc #13231
[breaking-change]
[RFC#3]

@@ -57,6 +57,8 @@ pub struct Arc<T> {
x: *mut ArcInner<T>,
}

impl<T> Share for Arc<T> {}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need T: Share? (And can't it be #[deriving]'d?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops, yes, it should be T: Share (in which case it can use the deriving attr).

Copy link
Member

Choose a reason for hiding this comment

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

It seems that most of these manual impls in this patch could be done by #[deriving]? (Also, what about built-in traits like Send?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I must have added them before the deriving patch landed. The work for Send and Copy will happen in a separate PR. I've already started working on it but it's easier to review / work on if we keep them separate.

Copy link
Member

Choose a reason for hiding this comment

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

Theoretically each additional trait is just removing it from type contents and adding the type_fulfills_TRAIT function (and calling it in the right place?)? (i.e it should be a very small patch?)

I'll defer to your experience in this code though.

Copy link
Member

Choose a reason for hiding this comment

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

(Actually, on second thoughts, IMO, it would be better to avoid #[deriving] for Arc, since it has a raw pointer.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the opt-in specific changes shouldn't be big. Adding a type_fulfills_X and trait related tests would be enough. What I can't estimate, though, is the fallout for each case. The biggest one seems to be Send.

Fallout changes are pretty isolated in per-module commits (as much as possible :D). However, it's probably easier from a reviewing/rebasing perspective to check/work on trait implementations if they are in separate reviews. TBH, I'm happy either way. I like the idea of this RFC landing all together, although that probably means it'll take longer and it'll be more painful rebase-wise. 😄

@brson
Copy link
Contributor

brson commented Jul 14, 2014

Closing due to inactivity. I think @nikomatsakis has some changes in mind for the design as well. Start this again later.

@brson brson closed this Jul 14, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 15, 2024
Preserve comments for extracted block expression in 'extract_function'

Fix rust-lang#14371

Preserve comments for extracted block expression in 'extract_function'.

In the original implementation, `block.statements()` was used to construct a new function, removing the comments within the block. In the updated implementation, we use manual traversal of nodes and `hacky_block_expr` to generate a new block, thereby preserving the comments.
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.

4 participants