Skip to content

Commit

Permalink
cleanup some code, adjust some comments
Browse files Browse the repository at this point in the history
  • Loading branch information
nikomatsakis committed Apr 10, 2015
1 parent e580df6 commit 3f310ae
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 45 deletions.
28 changes: 9 additions & 19 deletions src/librustc/middle/expr_use_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -848,26 +848,16 @@ impl<'d,'t,'tcx,TYPER:mc::Typer<'tcx>> ExprUseVisitor<'d,'t,'tcx,TYPER> {

self.walk_autoderefs(expr, adj.autoderefs);

// Weird hacky special case: AutoUnsizeUniq, which converts
// from a ~T to a ~Trait etc, always comes in a stylized
// fashion. In particular, we want to consume the ~ pointer
// being dereferenced, not the dereferenced content (as the
// content is, at least for upcasts, unsized).
if let Some(ty) = adj.unsize {
if let ty::ty_uniq(_) = ty.sty {
assert!(adj.autoderefs == 0,
format!("Expected no derefs with unsize AutoRefs, found: {}",
adj.repr(self.tcx())));
let cmt_unadjusted =
return_if_err!(self.mc.cat_expr_unadjusted(expr));
self.delegate_consume(expr.id, expr.span, cmt_unadjusted);
return;
}
}
let cmt_derefd =
return_if_err!(self.mc.cat_expr_autoderefd(expr, adj.autoderefs));

This comment has been minimized.

Copy link
@nrc

nrc Apr 10, 2015

This seems like it will duplicate the work done in walk_autoderefs to some extent - is that a problem?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 13, 2015

Author Owner

yes, it does (this is pre-existing). I decided it didn't matter enough but I agree it could be better.

This comment has been minimized.

Copy link
@nrc

nrc Apr 13, 2015

Ok, I was worried it would cause an error but it sounds like it won't, just duplicate effort


let cmt_derefd = return_if_err!(
self.mc.cat_expr_autoderefd(expr, adj.autoderefs));
self.walk_autoref(expr, cmt_derefd, adj.autoref);
let cmt_refd =
self.walk_autoref(expr, cmt_derefd, adj.autoref);

if adj.unsize.is_some() {
// Unsizing consumes the thin pointer and produces a fat one.
self.delegate_consume(expr.id, expr.span, cmt_refd);
}
}


Expand Down
77 changes: 52 additions & 25 deletions src/librustc/middle/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,51 +290,77 @@ pub enum AutoAdjustment<'tcx> {

/// Represents coercing a pointer to a different kind of pointer - where 'kind'
/// here means either or both of raw vs borrowed vs unique and fat vs thin.
/// The simplest cases are where the pointer is not adjusted fat vs thin. Here
/// the pointer will be dereferenced N times (where a dereference can happen to
/// to raw or borrowed pointers or any smart pointer which implements Deref,
/// including Box<_>). The number of dereferences is given by `autoderefs`.
/// It can then be auto-referenced zero or one times, indicated by `autoref`, to
/// We transform pointers by following the following steps in order:

This comment has been minimized.

Copy link
@nrc

nrc Apr 10, 2015

I don't get what you mean by "in order" here. This is I guess the literal order in which trans does the coercion? That is a little confusing because the type in the unsize field is pre-autoref, so the logical order is deref, unsize, ref, not deref, ref, unsize.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 13, 2015

Author Owner

Hmm, I don't think this is true:

the type in the unsize field is pre-autoref

if that were true, then this code would be wrong, because it never applies the autoref to the "unsize type".

This comment has been minimized.

Copy link
@nrc

nrc Apr 13, 2015

Yeah, you're right. I looked again at the coercion code, and it figures the type out pre-autoref, but then adjusts it (by wrapping it back in the pointer) before adding it to the adjustment. I had missed that second step before.

/// 1. Deref the pointer `self.autoderefs` times (may be 0).
/// 2. If `autoref` is `Some(_)`, then take the address and produce either a
/// `&` or `*` pointer.
/// 3. If `unsize` is `Some(_)`, then apply the unsize transformation,
/// which will do things like convert thin pointers to fat
/// pointers, or convert structs containing thin pointers to
/// structs containing fat pointers, or convert between fat
/// pointers. We don't store the details of how the transform is
/// done (in fact, we don't know that, because it might depend on
/// the precise type parameters). We just store the target
/// type. Trans figures out what has to be done at monomorphization
/// time based on the precise source/target type at hand.
///
/// To make that more concrete, here are some common scenarios:
///
/// 1. The simplest cases are where the pointer is not adjusted fat vs
/// thin. Here the pointer will be dereferenced N times (where a
/// dereference can happen to to raw or borrowed pointers or any smart
/// pointer which implements Deref, including Box<_>). The number of
/// dereferences is given by `autoderefs`. It can then be
/// auto-referenced zero or one times, indicated by `autoref`, to
/// either a raw or borrowed pointer. In these cases unsize is None.
///
/// A DST coercon involves unsizing the underlying data. We start with a thin
/// 2. A thin-to-fat coercon involves unsizing the underlying data. We start with a thin
/// pointer, deref a number of times, unsize the underlying data, then autoref.
/// The 'unsize' phase may change a fixed length array to a dynamically sized one,
/// a concrete object to a trait object, or statically sized struct to a dyncamically
/// sized one.
/// E.g., &[i32; 4] -> &[i32] is represented by:
/// sized one. E.g., &[i32; 4] -> &[i32] is represented by:
///
/// ```
/// AutoDerefRef {
/// autoderefs: 1, // &[i32; 4] -> [i32; 4]
/// unsize: Some([i32]), // [i32; 4] -> [i32]
/// autoref: Some(AutoPtr), // [i32] -> &[i32]
/// unsize: Some([i32]), // [i32; 4] -> [i32]

This comment has been minimized.

Copy link
@nrc

nrc Apr 10, 2015

I had this ordering of the struct fields to indicate the logical order of operation, rather than strictly reflecting the struct.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 13, 2015

Author Owner

yes, so did I :) I think we disagree about the logical order.

/// }
/// Note that for a struct, the 'deep' unsizing of the struct is not recorded.
/// E.g., `struct Foo<T> { x: T }` we can coerce &Foo<[i32; 4]> to &Foo<[i32]>
/// The autoderef and -ref are the same as in the above example, but the type
/// stored in `unsize` is `Foo<[i32]>`, we don't store any further detail about
/// the underlying conversions from `[i32; 4]` to `[i32]`.
/// ```
///
/// Box pointers are treated somewhat differently, the last deref is not counted,
/// nor is the 'ref' to a `Box<_>`. Imagine them more like structs.
/// E.g., Box<[i32; 4]> -> Box<[i32]> is represented by:
/// Note that for a struct, the 'deep' unsizing of the struct is not
/// recorded. E.g., `struct Foo<T> { x: T }` we can coerce &Foo<[i32;
/// 4]> to &Foo<[i32]> The autoderef and -ref are the same as in the

This comment has been minimized.

Copy link
@nrc

nrc Apr 10, 2015

Your word-wrap is being a bit too aggressive here.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 13, 2015

Author Owner

yeah, too bad there's no way to tell emacs to respect `` when word-wrapping...

/// above example, but the type stored in `unsize` is `Foo<[i32]>`, we
/// don't store any further detail about the underlying conversions
/// from `[i32; 4]` to `[i32]`.
///
/// 3. Coercing a `Box<T>` to `Box<Trait>` is an interesting special
/// case. In that case, we have the pointer we need coming in, so
/// there are no autoderefs, and no autoref. Instead we just do the
/// `Unsize` transformation. At some point, of course, `Box` should
/// move out of the compiler, in which case this is analogous to
/// transformating a struct. E.g., Box<[i32; 4]> -> Box<[i32]> is
/// represented by:
///
/// ```
/// AutoDerefRef {
/// autoderefs: 0,
/// unsize: Some(Box<[i32]>),
/// autoref: None,
/// unsize: Some(Box<[i32]>),
/// }
/// ```
#[derive(Copy, Clone, Debug)]
pub struct AutoDerefRef<'tcx> {
// FIXME with more powerful date structures we could have a better design
// here.

/// Apply a number of dereferences, producing an lvalue.
/// Step 1. Apply a number of dereferences, producing an lvalue.
pub autoderefs: usize,

/// Produce a pointer/reference from the value.
/// Step 2. Optionally produce a pointer/reference from the value.
pub autoref: Option<AutoRef<'tcx>>,

/// Unsize a pointer/reference value, e.g. &[T; n] to &[T].
/// The stored type is the target pointer type.
/// Step 3. Unsize a pointer/reference value, e.g. `&[T; n]` to
/// `&[T]`. The stored type is the target pointer type. Note that
/// the source could be a thin or fat pointer.

This comment has been minimized.

Copy link
@nrc

nrc Apr 10, 2015

I find the step numbering a bit odd - in the &thin to &fat case, step 3 happens before step 2.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 13, 2015

Author Owner

as above, I think that's actually not the case -- neither logically nor in actuality.

This comment has been minimized.

Copy link
@nrc

nrc Apr 13, 2015

I think that is the case when checking for coercions during type checking, but not in trans (in both cases, logically and actually). I.e., you are right from the perspective of the code which is generated, but not from the perspective of how the adjustment itself is created. I tend to think of these adjustments from the latter perspective, but perhaps that is not so helpful.

pub unsize: Option<Ty<'tcx>>,
}

Expand Down Expand Up @@ -5831,6 +5857,7 @@ pub fn struct_lockstep_tails<'tcx>(cx: &ctxt<'tcx>,
source: Ty<'tcx>,
target: Ty<'tcx>)
-> (Ty<'tcx>, Ty<'tcx>) {
// NIT: Maybe move `struct_lockstep_tails` into trans, since it is only used there?

This comment has been minimized.

Copy link
@nrc

nrc Apr 10, 2015

I imagine its here so it can be next to struct_tail, which makes sense. Moving to trans also makes sense, but I imagine this could be useful outside trans.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 13, 2015

Author Owner

Yeah, maybe. I tend to put fns in the narrowest place possible and move it later if we need it elsewhere...mostly this is because it helps me when reading code. When I encounter this fn here, I am inclined to wonder where it is used, and have to run a ack through the codebase to find out. But if I see it in trans, I know it is specific to there.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 13, 2015

Author Owner

(But I don't really care -- this is a fine place too.)

let (mut a, mut b) = (source, target);
while let (&ty_struct(a_did, a_substs), &ty_struct(b_did, b_substs)) = (&a.sty, &b.sty) {
if a_did != b_did {
Expand Down
5 changes: 4 additions & 1 deletion src/librustc_typeck/check/method/confirm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,10 @@ impl<'a,'tcx> ConfirmContext<'a,'tcx> {
ty::adjust_ty_for_autoref(self.tcx(), target, Some(autoref))
}))
} else {
// No unsizing should be performed without autoref.
// No unsizing should be performed without autoref (at
// least during method dispach). This is because we
// currently only unsize `[T;N]` to `[T]`, and naturally
// that must occur being a reference.
assert!(pick.unsize.is_none());
(None, None)
};
Expand Down

0 comments on commit 3f310ae

Please sign in to comment.