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

Allow slices to be used in field #46445

Closed
wants to merge 1 commit into from

Conversation

GuillaumeGomez
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2017
@Mark-Simulacrum
Copy link
Member

I'm not sure that these changes are backwards-compatible, and also seem to go against the reason for using a trait object (making the output code small). Why are we making this change? What specifically does it allow; what are the tradeoffs?

Overall, I don't think I'm the best reviewer for this, maybe r? @dtolnay

Looks like Travis failed, too.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

For the gstreamer-sys change, would it work to use &&self.v[..] instead of &self.v as &[i32]? Playground

@GuillaumeGomez
Copy link
Member Author

@dtolnay: it would indeed, but sounds more like a trick. :-/ At least, with this, it'd be more "logical" (if we could combine trait objects, it'd be even simpler).

@Mark-Simulacrum: It fails because a test supposed to fail is now passing. Also, I have no issue having a debate over this PR. It was just a fix for a specific situation after all.

@dtolnay
Copy link
Member

dtolnay commented Dec 4, 2017

I would prefer to stick with &&self.v[..] for now and follow up with the real fix which is allowing dynamically sized types to be trait objects. That may allow &self.v as &[i32] without this change. All of std::fmt uses trait objects extensively to help compile times and executable sizes.

@GuillaumeGomez
Copy link
Member Author

That'd be awesome! Any follow-up issue for dynamically sized trait objects?

@dtolnay
Copy link
Member

dtolnay commented Dec 7, 2017

Hmm, it may be rust-lang/rfcs#2035 with something like &(Debug + ?Sized) but I'm not sure.

@dtolnay
Copy link
Member

dtolnay commented Dec 8, 2017

Another possible fix would be a deref coercion where &DynamicallySizedType coerces to &TraitObject by implicitly doing & twice.

use std::fmt::Debug;

fn f(_trait_object: &Debug) {}

fn main() {
    f("str"); // does not compile today. would coerce to &&str
    f(&[1][..]); // does not compile today. would coerce to &&[{integer}]
}

That would also make &self.v as &[i32] work without changing the signature of DebugStruct::field. I filed rust-lang/rfcs#2239 to follow up.

@dtolnay dtolnay closed this Dec 8, 2017
@GuillaumeGomez GuillaumeGomez deleted the better-field branch December 9, 2017 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants