Auto merge of #105 - emilio:msvc, r=fitzgen,emilio

Pass the potential id to be used as the type wrapper id, and prevent leaving a dangling a type parsing template aliases.

This should fix the MSVC problems @upsuper has been having.

@upsuper, can you confirm if this fixes the panic you were hitting?

Author: bors-servo

src/ir/context.rs

@@ -467,22 +467,22 @@ impl<'ctx> BindgenContext<'ctx> {
     ///     };
     /// ```
     fn build_template_wrapper(&mut self,
+                              with_id: ItemId,
                               wrapping: ItemId,
                               parent_id: ItemId,
                               ty: &clang::Type,
                               location: clang::Cursor) -> ItemId {
         use clangll::*;
         let mut args = vec![];
         let mut found_invalid_template_ref = false;
-        let self_id = ItemId::next();
         location.visit(|c, _| {
             if c.kind() == CXCursor_TemplateRef &&
                c.cur_type().kind() == CXType_Invalid {
                 found_invalid_template_ref = true;
             }
             if c.kind() == CXCursor_TypeRef {
                 let new_ty =
-                    Item::from_ty_or_ref(c.cur_type(), Some(*c), Some(self_id), self);
+                    Item::from_ty_or_ref(c.cur_type(), Some(*c), Some(with_id), self);
                 args.push(new_ty);
             }
             CXChildVisit_Continue
@@ -528,17 +528,18 @@ impl<'ctx> BindgenContext<'ctx> {
             let name = ty.spelling();
             let name = if name.is_empty() { None } else { Some(name) };
             let ty = Type::new(name, ty.fallible_layout().ok(), type_kind, ty.is_const());
-            Item::new(self_id, None, None, parent_id, ItemKind::Type(ty))
+            Item::new(with_id, None, None, parent_id, ItemKind::Type(ty))
         };
 
         // Bypass all the validations in add_item explicitly.
-        self.items.insert(self_id, item);
-        self_id
+        self.items.insert(with_id, item);
+        with_id
     }
 
     /// Looks up for an already resolved type, either because it's builtin, or
     /// because we already have it in the map.
     pub fn builtin_or_resolved_ty(&mut self,
+                                  with_id: ItemId,
                                   parent_id: Option<ItemId>,
                                   ty: &clang::Type,
                                   location: Option<clang::Cursor>) -> Option<ItemId> {
@@ -567,6 +568,7 @@ impl<'ctx> BindgenContext<'ctx> {
             if let Some(id) = id {
                 debug!("Already resolved ty {:?}, {:?}, {:?} {:?}",
                        id, declaration, ty, location);
+
                 // If the declaration existed, we *might* be done, but it's not
                 // the case for class templates, where the template arguments
                 // may vary.
@@ -582,11 +584,12 @@ impl<'ctx> BindgenContext<'ctx> {
                    *ty != canonical_declaration.cur_type() &&
                    location.is_some() && parent_id.is_some() {
                     return Some(
-                        self.build_template_wrapper(id, parent_id.unwrap(), ty,
+                        self.build_template_wrapper(with_id, id,
+                                                    parent_id.unwrap(), ty,
                                                     location.unwrap()));
                 }
 
-                return Some(self.build_ty_wrapper(id, parent_id, ty));
+                return Some(self.build_ty_wrapper(with_id, id, parent_id, ty));
             }
         }
 
@@ -602,19 +605,19 @@ impl<'ctx> BindgenContext<'ctx> {
     // We should probably make the constness tracking separate, so it doesn't
     // bloat that much, but hey, we already bloat the heck out of builtin types.
     fn build_ty_wrapper(&mut self,
+                        with_id: ItemId,
                         wrapped_id: ItemId,
                         parent_id: Option<ItemId>,
                         ty: &clang::Type) -> ItemId {
-        let id = ItemId::next();
         let spelling = ty.spelling();
         let is_const = ty.is_const();
         let layout = ty.fallible_layout().ok();
         let type_kind = TypeKind::ResolvedTypeRef(wrapped_id);
         let ty = Type::new(Some(spelling), layout, type_kind, is_const);
-        let item = Item::new(id, None, None,
+        let item = Item::new(with_id, None, None,
                              parent_id.unwrap_or(self.current_module), ItemKind::Type(ty));
         self.add_builtin_item(item);
-        id
+        with_id
     }
 
     fn build_builtin_ty(&mut self,

src/ir/item.rs

@@ -7,7 +7,7 @@ use super::ty::{Type, TypeKind};
 use super::function::Function;
 use super::module::Module;
 use super::annotations::Annotations;
-use std::cell::Cell;
+use std::cell::{Cell, RefCell};
 use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};
 use parse::{ClangItemParser, ClangSubItemParser, ParseError, ParseResult};
 use clang;
@@ -97,11 +97,25 @@ impl ItemCanonicalPath for ItemId {
 pub struct Item {
     /// This item's id.
     id: ItemId,
-    /// The item's local id, unique only amongst its siblings.  Only used
-    /// for anonymous items.  Lazily initialized in local_id().
+
+    /// The item's local id, unique only amongst its siblings.  Only used for
+    /// anonymous items.
+    ///
+    /// Lazily initialized in local_id().
+    ///
+    /// Note that only structs, unions, and enums get a local type id. In any
+    /// case this is an implementation detail.
     local_id: Cell<Option<usize>>,
-    /// The next local id to use for a child.
+
+    /// The next local id to use for a child..
     next_child_local_id: Cell<usize>,
+
+    /// A cached copy of the canonical name, as returned by `canonical_name`.
+    ///
+    /// This is a fairly used operation during codegen so this makes bindgen
+    /// considerably faster in those cases.
+    canonical_name_cache: RefCell<Option<String>>,
+
     /// A doc comment over the item, if any.
     comment: Option<String>,
     /// Annotations extracted from the doc comment, or the default ones
@@ -129,6 +143,7 @@ impl Item {
             id: id,
             local_id: Cell::new(None),
             next_child_local_id: Cell::new(1),
+            canonical_name_cache: RefCell::new(None),
             parent_id: parent_id,
             comment: comment,
             annotations: annotations.unwrap_or_default(),
@@ -536,6 +551,10 @@ impl Item {
                 _ => {}
             }
         }
+
+        // Note that this `id_` prefix prevents (really unlikely) collisions
+        // between the global id and the local id of an item with the same
+        // parent.
         format!("id_{}", self.id().0)
     }
 
@@ -717,7 +736,9 @@ impl ClangItemParser for Item {
                         .expect("Unable to resolve type");
         }
 
-        if let Some(ty) = context.builtin_or_resolved_ty(parent_id, &ty, location) {
+        if let Some(ty) = context.builtin_or_resolved_ty(potential_id,
+                                                         parent_id, &ty,
+                                                         location) {
             debug!("{:?} already resolved: {:?}", ty, location);
             return ty;
         }
@@ -779,7 +800,8 @@ impl ClangItemParser for Item {
             context.replace(replaced, id);
         }
 
-        if let Some(ty) = context.builtin_or_resolved_ty(parent_id, ty, location) {
+        if let Some(ty) = context.builtin_or_resolved_ty(id, parent_id,
+                                                         ty, location) {
             return Ok(ty);
         }
 
@@ -917,7 +939,13 @@ impl ItemCanonicalName for Item {
         if let Some(other_canon_type) = self.annotations.use_instead_of() {
             return other_canon_type.to_owned();
         }
-        self.real_canonical_name(ctx, ctx.options().enable_cxx_namespaces, false)
+        if self.canonical_name_cache.borrow().is_none() {
+            *self.canonical_name_cache.borrow_mut() =
+                Some(self.real_canonical_name(ctx,
+                                              ctx.options().enable_cxx_namespaces,
+                                              false));
+        }
+        return self.canonical_name_cache.borrow().as_ref().unwrap().clone();
     }
 }
 

src/ir/ty.rs

@@ -471,7 +471,8 @@ impl Type {
                          parent_id: Option<ItemId>,
                          ctx: &mut BindgenContext) -> Result<ParseResult<Self>, ParseError> {
         use clangll::*;
-        if let Some(ty) = ctx.builtin_or_resolved_ty(parent_id, ty, location) {
+        if let Some(ty) = ctx.builtin_or_resolved_ty(potential_id, parent_id,
+                                                     ty, location) {
             debug!("{:?} already resolved: {:?}", ty, location);
             return Ok(ParseResult::AlreadyResolved(ty));
         }
@@ -565,11 +566,17 @@ impl Type {
                                 return Err(ParseError::Continue);
                             }
 
-                            if args.is_empty() {
-                                error!("Failed to get any template parameter, maybe a specialization? {:?}", location);
-                                return Err(ParseError::Continue);
-                            }
-
+                            // NB: `args` may be empty here (if for example the
+                            // template parameters are constants).
+                            //
+                            // We can't reject it here then because inner points
+                            // to `potential_id` now, so either we remove
+                            // `inner` and return an error, or carry on.
+                            //
+                            // In this case, we just carry on, since it seems
+                            // easier if than removing every possible reference
+                            // to `item` from `ctx`, and it doesn't give any
+                            // problems that we didn't have anyway.
                             TypeKind::TemplateAlias(inner.unwrap(), args)
                         }
                         CXCursor_TemplateRef => {

tests/expectations/elaborated.rs

@@ -0,0 +1,11 @@
+/* automatically generated by rust-bindgen */
+
+
+#![allow(non_snake_case)]
+
+
+pub type whatever_t = ::std::os::raw::c_int;
+extern "C" {
+    #[link_name = "_Z9somethingPKi"]
+    pub fn something(wat: *const whatever_t);
+}

tests/expectations/empty_template_param_name.rs

@@ -4,6 +4,7 @@
 #![allow(non_snake_case)]
 
 
+pub type __void_t = ::std::os::raw::c_void;
 #[repr(C)]
 #[derive(Debug, Copy, Clone)]
 pub struct __iterator_traits<_Iterator> {

tests/expectations/type_alias_empty.rs

@@ -0,0 +1,7 @@
+/* automatically generated by rust-bindgen */
+
+
+#![allow(non_snake_case)]
+
+
+

tests/headers/elaborated.hpp

@@ -0,0 +1,5 @@
+namespace whatever {
+  typedef int whatever_t;
+}
+
+void something(const whatever::whatever_t *wat);

tests/headers/type_alias_empty.hpp

@@ -0,0 +1,10 @@
+// bindgen-flags: --whitelist-type bool_constant -- -std=c++11
+
+// NB: The --whitelist-type is done to trigger the traversal of the types on
+// codegen in order to trigger #67.
+
+template<typename T, T Val>
+struct integral_constant {};
+
+template<bool B>
+using bool_constant = integral_constant<bool, B>;