Auto merge of #99 - emilio:docs-for-real, r=fitzgen

ir: A bit more documentation and cleanup in parts of the `ir` module.

r? @fitzgen or @nox

Author: bors-servo

src/ir/item.rs

@@ -42,7 +42,7 @@ pub trait ItemCanonicalPath {
 
 /// A single identifier for an item.
 ///
-/// TODO: Build stronger abstractions on top of this, like TypeId(ItemId), ...
+/// TODO: Build stronger abstractions on top of this, like TypeId(ItemId)?
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct ItemId(usize);
 
@@ -78,6 +78,20 @@ impl ItemCanonicalPath for ItemId {
     }
 }
 
+/// An item is the base of the bindgen representation, it can be either a
+/// module, a type, a function, or a variable (see `ItemKind` for more
+/// information).
+///
+/// Items form a tree, and each item only stores the id of the parent.
+///
+/// The root of this tree is the "root module", a meta-item used to hold all the
+/// top-level items.
+///
+/// An item may have a comment, and annotations (see the `annotations` module).
+///
+/// Note that even though we parse all the types of annotations in comments, not
+/// all of them apply to every item. Those rules are described in the
+/// `annotations` module.
 #[derive(Debug)]
 pub struct Item {
     /// This item's id.
@@ -133,6 +147,24 @@ impl Item {
         &mut self.kind
     }
 
+    /// Returns whether this item is a top-level item, from the point of view of
+    /// bindgen.
+    ///
+    /// This point of view changes depending on whether namespaces are enabled
+    /// or not. That way, in the following example:
+    ///
+    /// ```c++
+    /// namespace foo {
+    ///     static int var;
+    /// }
+    /// ```
+    ///
+    /// `var` would be a toplevel item if namespaces are disabled, but won't if
+    /// they aren't.
+    ///
+    /// This function is used to determine when the codegen phase should call
+    /// `codegen` on an item, since it's assumed that any item that is not
+    /// top-level will be generated by its parent.
     pub fn is_toplevel(&self, ctx: &BindgenContext) -> bool {
         // FIXME: Workaround for some types falling behind when parsing weird
         // stl classes, for example.
@@ -167,12 +199,55 @@ impl Item {
         self.kind().expect_function()
     }
 
-    // This check is needed because even though the type might not contain the
-    // applicable template args itself, they might apply transitively via, for
-    // example, the parent.
-    //
-    // It's kind of unfortunate (in the sense that it's a sort of complex
-    // process, but I think it gets all the cases).
+    /// Checks whether an item contains in its "type signature" some named type.
+    ///
+    /// This function is used to avoid unused template parameter errors in Rust
+    /// when generating typedef declarations, and also to know whether we need
+    /// to generate a PhantomData member for a template parameter.
+    ///
+    /// For example, in code like the following:
+    ///
+    /// ```c++
+    /// template<typename T, typename U>
+    /// struct Foo {
+    ///     T bar;
+    ///
+    ///     struct Baz {
+    ///         U bas;
+    ///     };
+    /// };
+    /// ```
+    ///
+    /// Both Foo and Baz contain both `T` and `U` template parameters in their
+    /// signature:
+    ///
+    ///  * `Foo<T, U>`
+    ///  * `Bar<T, U>`
+    ///
+    /// But the structure for `Foo` would look like:
+    ///
+    /// ```rust
+    /// struct Foo<T, U> {
+    ///     bar: T,
+    ///     _phantom0: ::std::marker::PhantomData<U>,
+    /// }
+    /// ```
+    ///
+    /// because non of its member fields contained the `U` type in the
+    /// signature. Similarly, `Bar` would contain a `PhantomData<T>` type, for
+    /// the same reason.
+    ///
+    /// Note that this is somewhat similar to `applicable_template_args`, but
+    /// this also takes into account other kind of types, like arrays,
+    /// (`[T; 40]`), pointers: `*mut T`, etc...
+    ///
+    /// Normally we could do this check just in the `Type` kind, but we also
+    /// need to check the `applicable_template_args` more generally, since we
+    /// could need a type transitively from our parent, see the test added in
+    /// <https://github.com/servo/rust-bindgen/pull/85/commits/2a3f93074dd2898669dbbce6e97e5cc4405d7cb1>
+    ///
+    /// It's kind of unfortunate (in the sense that it's a sort of complex
+    /// process), but I think it should get all the cases.
     fn signature_contains_named_type(&self, ctx: &BindgenContext, ty: &Type) -> bool {
         debug_assert!(ty.is_named());
         self.expect_type().signature_contains_named_type(ctx, ty) ||
@@ -181,6 +256,39 @@ impl Item {
             })
     }
 
+    /// Returns the template arguments that apply to a struct. This is a concept
+    /// needed because of type declarations inside templates, for example:
+    ///
+    /// ```c++
+    /// template<typename T>
+    /// class Foo {
+    ///     typedef T element_type;
+    ///     typedef int Bar;
+    ///
+    ///     template<typename U>
+    ///     class Baz {
+    ///     };
+    /// };
+    /// ```
+    ///
+    /// In this case, the applicable template arguments for the different types
+    /// would be:
+    ///
+    ///  * `Foo`: [`T`]
+    ///  * `Foo::element_type`: [`T`]
+    ///  * `Foo::Bar`: [`T`]
+    ///  * `Foo::Baz`: [`T`, `U`]
+    ///
+    /// You might notice that we can't generate something like:
+    ///
+    /// ```rust,ignore
+    /// type Foo_Bar<T> = ::std::os::raw::c_int;
+    /// ```
+    ///
+    /// since that would be invalid Rust. Still, conceptually, `Bar` *could* use
+    /// the template parameter type `T`, and that's exactly what this method
+    /// represents. The unused template parameters get stripped in the
+    /// `signature_contains_named_type` check.
     pub fn applicable_template_args(&self, ctx: &BindgenContext) -> Vec<ItemId> {
         let ty = match *self.kind() {
             ItemKind::Type(ref ty) => ty,
@@ -275,6 +383,15 @@ impl Item {
 
     /// Get the canonical name without taking into account the replaces
     /// annotation.
+    ///
+    /// This is the base logic used to implement hiding and replacing via
+    /// annotations, and also to implement proper name mangling.
+    ///
+    /// The idea is that each generated type in the same "level" (read: module
+    /// or namespace) has a unique canonical name.
+    ///
+    /// This name should be derived from the immutable state contained in the
+    /// type and the parent chain, since it should be consistent.
     fn real_canonical_name(&self,
                            ctx: &BindgenContext,
                            count_namespaces: bool,
@@ -424,12 +541,6 @@ impl ClangItemParser for Item {
         let comment = cursor.raw_comment();
         let annotations = Annotations::new(&cursor);
 
-        // FIXME: The current_module logic is not really accurate. We should be
-        // able to index modules by their Cursor, and locate the proper module
-        // for a given item.
-        //
-        // We don't support modules properly though, so there's no rush for
-        // this.
         let current_module = context.current_module();
         macro_rules! try_parse {
             ($what:ident) => {
@@ -486,7 +597,8 @@ impl ClangItemParser for Item {
         if cursor.kind() == clangll::CXCursor_UnexposedDecl {
             Err(ParseError::Recurse)
         } else {
-            error!("Unhandled cursor kind: {} ({})", ::clang::kind_to_str(cursor.kind()), cursor.kind());
+            error!("Unhandled cursor kind: {} ({})",
+                   ::clang::kind_to_str(cursor.kind()), cursor.kind());
             Err(ParseError::Continue)
         }
     }
@@ -498,6 +610,17 @@ impl ClangItemParser for Item {
         Self::from_ty_or_ref_with_id(ItemId::next(), ty, location, parent_id, context)
     }
 
+    /// Parse a type, if we know it before hand, or otherwise store it as an
+    /// `UnresolvedTypeRef`, which means something like "a reference to a type
+    /// we still don't know".
+    ///
+    /// This logic is needed to avoid parsing items with the incorrect parent
+    /// and it's sort of complex to explain, so I'll just point to
+    /// `tests/headers/typeref.hpp` to see the kind of constructs that forced
+    /// this.
+    ///
+    /// Typerefs are resolved once parsing is completely done, see
+    /// `BindgenContext::resolve_typerefs`.
     fn from_ty_or_ref_with_id(potential_id: ItemId,
                               ty: clang::Type,
                               location: Option<clang::Cursor>,
@@ -537,6 +660,14 @@ impl ClangItemParser for Item {
         Self::from_ty_with_id(ItemId::next(), ty, location, parent_id, context)
     }
 
+    /// This is one of the trickiest methods you'll find (probably along with
+    /// some of the ones that handle templates in `BindgenContext`).
+    ///
+    /// This method parses a type, given the potential id of that type (if
+    /// parsing it was correct), an optional location we're scanning, which is
+    /// critical some times to obtain information, an optional parent item id,
+    /// that will, if it's `None`, become the current module id, and the
+    /// context.
     fn from_ty_with_id(id: ItemId,
                        ty: &clang::Type,
                        location: Option<clang::Cursor>,

src/ir/item_kind.rs

@@ -15,6 +15,7 @@ pub enum ItemKind {
 
     /// A function or method declaration.
     Function(Function),
+
     /// A variable declaration, most likely a static.
     Var(Var),
 }

src/ir/mod.rs

@@ -1,3 +1,6 @@
+//! The module where the Intermediate Representation bindgen uses, and the
+//! parsing code that generates it lives.
+
 pub mod annotations;
 pub mod comp;
 pub mod context;