From e13111fc5ab9a968d5668712bb75bc49350a7f25 Mon Sep 17 00:00:00 2001
From: Jed Davis <jld@panix.com>
Date: Sat, 2 Mar 2013 14:03:41 -0800
Subject: [PATCH] Even more comments for ADT-related interfaces

---
 src/librustc/middle/trans/adt.rs  | 26 +++++++++++++++++++++++---
 src/librustc/middle/trans/expr.rs | 26 +++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/src/librustc/middle/trans/adt.rs b/src/librustc/middle/trans/adt.rs
index 98f15290470..e61925395f2 100644
--- a/src/librustc/middle/trans/adt.rs
+++ b/src/librustc/middle/trans/adt.rs
@@ -394,9 +394,20 @@ pub fn trans_drop_flag_ptr(bcx: block, r: &Repr, val: ValueRef) -> ValueRef {
  * alignment!) from the representation's `type_of`, so it needs a
  * pointer cast before use.
  *
- * Currently it has the same size as the type, but this may be changed
- * in the future to avoid allocating unnecessary space after values of
- * shorter-than-maximum cases.
+ * The LLVM type system does not directly support unions, and only
+ * pointers can be bitcast, so a constant (and, by extension, the
+ * GlobalVariable initialized by it) will have a type that can vary
+ * depending on which case of an enum it is.
+ *
+ * To understand the alignment situation, consider `enum E { V64(u64),
+ * V32(u32, u32) }` on win32.  The type should have 8-byte alignment
+ * to accommodate the u64 (currently it doesn't; this is a known bug),
+ * but `V32(x, y)` would have LLVM type `{i32, i32, i32}`, which is
+ * 4-byte aligned.
+ *
+ * Currently the returned value has the same size as the type, but
+ * this may be changed in the future to avoid allocating unnecessary
+ * space after values of shorter-than-maximum cases.
  */
 pub fn trans_const(ccx: @CrateContext, r: &Repr, discr: int,
                    vals: &[ValueRef]) -> ValueRef {
@@ -424,6 +435,9 @@ pub fn trans_const(ccx: @CrateContext, r: &Repr, discr: int,
             let max_sz = cases.map(|s| s.size).max();
             let body = build_const_struct(ccx, case, vals);
 
+            // The unary packed struct has alignment 1 regardless of
+            // its contents, so it will always be located at the
+            // expected offset at runtime.
             C_struct([C_int(ccx, discr),
                       C_packed_struct([C_struct(body)]),
                       padding(max_sz - case.size)])
@@ -434,6 +448,12 @@ pub fn trans_const(ccx: @CrateContext, r: &Repr, discr: int,
 /**
  * Building structs is a little complicated, because we might need to
  * insert padding if a field's value is less aligned than its type.
+ *
+ * Continuing the example from `trans_const`, a value of type `(u32,
+ * E)` should have the `E` at offset 8, but if that field's
+ * initializer is 4-byte aligned then simply translating the tuple as
+ * a two-element struct will locate it at offset 4, and accesses to it
+ * will read the wrong memory.
  */
 fn build_const_struct(ccx: @CrateContext, st: &Struct, vals: &[ValueRef])
     -> ~[ValueRef] {
diff --git a/src/librustc/middle/trans/expr.rs b/src/librustc/middle/trans/expr.rs
index edda2f6c2c1..699f73e7d77 100644
--- a/src/librustc/middle/trans/expr.rs
+++ b/src/librustc/middle/trans/expr.rs
@@ -1158,9 +1158,9 @@ fn trans_rec_or_struct(bcx: block,
         let mut need_base = vec::from_elem(field_tys.len(), true);
 
         let numbered_fields = do fields.map |field| {
-            match do vec::position(field_tys) |field_ty| {
-                field_ty.ident == field.node.ident
-            } {
+            let opt_pos = vec::position(field_tys, |field_ty|
+                                        field_ty.ident == field.node.ident);
+            match opt_pos {
                 Some(i) => {
                     need_base[i] = false;
                     (i, field.node.expr)
@@ -1196,11 +1196,31 @@ fn trans_rec_or_struct(bcx: block,
     }
 }
 
+/**
+ * Information that `trans_adt` needs in order to fill in the fields
+ * of a struct copied from a base struct (e.g., from an expression
+ * like `Foo { a: b, ..base }`.
+ *
+ * Note that `fields` may be empty; the base expression must always be
+ * evaluated for side-effects.
+ */
 struct StructBaseInfo {
+    /// The base expression; will be evaluated after all explicit fields.
     expr: @ast::expr,
+    /// The indices of fields to copy paired with their types.
     fields: ~[(uint, ty::t)]
 }
 
+/**
+ * Constructs an ADT instance:
+ *
+ * - `fields` should be a list of field indices paired with the
+ * expression to store into that field.  The initializers will be
+ * evaluated in the order specified by `fields`.
+ *
+ * - `optbase` contains information on the base struct (if any) from
+ * which remaining fields are copied; see comments on `StructBaseInfo`.
+ */
 fn trans_adt(bcx: block, repr: &adt::Repr, discr: int,
              fields: &[(uint, @ast::expr)],
              optbase: Option<StructBaseInfo>,