All the template parameters!! #544

fitzgen · 2017-02-27T18:22:50Z

The major changes in this PR are:

De-duplication of named template type parameters (this is probably the biggest part of the PR, and nastiest because it is the part that deals with libclang)
Removing the signature_contains_named_type stuff, and enabling the more sound template type parameter usage analysis that has been landing in bits here and there.
LOTS of new tests for template type parameter usage

@emilio: can you also test this on the stylo bindings? I tested on the SpiderMonkey bindings and found a bug and fixed a bug related to instantiations of partially specialized templates. I'd like to make sure that there aren't any latent bugs uncovered in Stylo.

This is NOT ready to merge quite yet, but is ready for some more eyeballs that are not mine.

Still TODO:

Rebase so that these changes will merge cleanly -- I'll get on this ASAP
Time SpiderMonkey bindings generation with and without these changes to see what the overhead of the new analysis is (if any!) on Large and Real World bindings
Test theses changes on Stylo (thanks @emilio!)
- (optional) and time Stylo bindings generation with and without these changes as well (only if you want to, @emilio)

Thanks!

highfive · 2017-02-27T18:22:53Z

Warning

These commits modify unsafe code. Please review it carefully!

fitzgen · 2017-02-27T18:44:21Z

Rebased!

emilio

Just a partial review because your new push makes github complain when I comment with "The commit is not part of the pull request".

emilio · 2017-02-27T18:37:23Z

src/ir/context.rs

+    /// template usage information is only computed as we enter the codegen
+    /// phase.
+    pub fn uses_template_parameter(&self, item: ItemId, template_param: ItemId) -> bool {
+        assert!(self.in_codegen_phase(),


Perhaps we should move this to something like a Phase enum?

enum Phase { Parsing, ReplacementProcessing, Codegen, }

With something like:

impl BindgenContext { pub fn advance_phase(&mut self, to: Phase) { assert!(to > self.current_phase); self.current_phase = to; } }

And make more fine-grained assertions?

It's probably fine as a followup, or even an easy bug. Do you think it's worth filing?

I think even better would be to have different Context types and get rid of that nasty transmute. Then we could simply have methods that require more info on the context that has that info by construction, and push all these asserts into the type system at compile time.

That sounds fine, though it's obviously a bigger change ;)

Yeah, but also something we need pretty badly IMO. The transmuted lifetime can make it easy to accidentally have conflicting lifetimes for no good reason. I spent probably a half hour to an hour trying to get the named template parameter analysis working with the made up lifetime :-/

emilio · 2017-02-27T18:41:46Z

src/ir/ty.rs

+    /// Get only the set of template parameters that this item uses. This is a
+    /// subset of `all_template_params` and does not necessarily contain any of
+    /// `self_template_params`.
+    fn used_template_params(&self, ctx: &BindgenContext) -> Option<Vec<ItemId>>


Is the option needed? I believe in most cases it'd be more convenient to just return a Vec<ItemId>, and make an empty vector represent the lack of them?

The idea is to differentiate between these cases:

template <> class IsTemplateWithNoParams {}; class IsNotTemplate {};

I don't know if it is worth it however, as we eventually treat them identically.

I guess that's fair, maybe worth mentioning this bit in the comment?

Can do, will wait for your complete review before I start changing anything.

This is a sorta-hot call already. I've noticed rust-lang#544 wants to add more assertions about this, and was going to suggest moving them to `debug_assert!`. But there's an easier way :) Signed-off-by: Emilio Cobos Álvarez <[email protected]>

fitzgen · 2017-02-27T19:07:51Z

Looks like I need to disable the last commit's new test on libclang <= 3.8:

 #[repr(C)]
 #[derive(Debug, Default, Copy)]
 pub struct Usage {
     pub _address: u8,
 }
 extern "C" {
     #[link_name = "_ZN5Usage13static_memberE"]
     pub static mut Usage_static_member: [u32; 2usize];
 }
 #[test]
 fn bindgen_test_layout_Usage() {
     assert_eq!(::std::mem::size_of::<Usage>() , 1usize , concat ! (
                "Size of: " , stringify ! ( Usage ) ));
     assert_eq! (::std::mem::align_of::<Usage>() , 1usize , concat ! (
                 "Alignment of " , stringify ! ( Usage ) ));
 }
+extern "C" {
+    #[link_name = "_ZN5UsageC1Ev"]
+    pub fn Usage_Usage(this: *mut Usage);
+}
 impl Clone for Usage {
     fn clone(&self) -> Self { *self }
+}
+impl Usage {
+    #[inline]
+    pub unsafe fn new() -> Self {
+        let mut __bindgen_tmp = ::std::mem::uninitialized();
+        Usage_Usage(&mut __bindgen_tmp);
+        __bindgen_tmp
+    }
 }
thread 'header_partial_specialization_and_inheritance_hpp' panicked at 'Header and binding differ!', /tmp/bindgen/debug/build/bindgen-e852d23f6698dc85/out/tests.rs:210
note: Run with `RUST_BACKTRACE=1` for a backtrace.

fitzgen · 2017-02-27T19:08:34Z

(It the lack of seeing that the constructor is inline, but we need the constructor to trigger the panic that I was reducing when I generated this test case)

@fitzgen

clang: Avoid a hot FFI call for getting the cursor kind. This is a sorta-hot call already. I've noticed #544 wants to add more assertions about this, and was going to suggest moving them to `debug_assert!`. But there's an easier way :) r? @fitzgen

fitzgen · 2017-02-27T19:16:28Z

Timing SpiderMonkey Bindings Generation

time	master	with these changes
real	9.50	9.24
user	9.35	9.10
sys	0.11	0.10

emilio

I've only skipped the last commit because I need to run. I'm super excited to see this, and it looks quite awesome so far!

I have a few comments here and there. My main concern is that the actual unused template parameter can be pretty useful when writing abstractions on top of the bindgen-generated code.

For example, in Gecko we have the FramePropertyDescriptor<T> type, which is used only for querying a property in a typed fashion, even though it doesn't store anything related to T. It'd be a bit of a shame to not generate T, since it's the only purpose of it (not that we need it so far, but it's an example).

It seems (from the reviewed code experience, which might not be so much) that switching from this code generation to the alternative (what we have now, template params + PhantomData) shouldn't be extremely hard (of course, it'd bring back the issues we have with template aliases). Is that right?

emilio · 2017-02-27T18:45:57Z

src/ir/comp.rs

@@ -524,7 +524,7 @@ impl CompInfo {
                        specialization = false;
                        None
                    } else {
-                        Some(Item::from_ty_or_ref(t, None, None, ctx))
+                        Some(Item::from_ty_or_ref(t, t.declaration(), None, ctx))


We may need the definition in order for it to be actually useful? It doesn't seem to matter here though.

emilio · 2017-02-27T18:46:51Z

src/ir/context.rs

@@ -103,6 +103,10 @@ pub struct BindgenContext<'ctx> {
    /// item ids during parsing.
    types: HashMap<TypeKey, ItemId>,

+    /// Maps from a cursor to the item id of the named template type parameter
+    /// for that cursor.
+    named_types: HashMap<clang::Cursor, ItemId>,


What do you think about moving most of our HashMap/HashSets to fnv? Of course a followup/E-easy is fine.

Seems like a good follow up. We also need to test whether it has any observable effect.

emilio · 2017-02-27T18:47:44Z

src/ir/context.rs

+               definition);
+
+        assert!(item.is_named());
+        assert!(definition.kind() == clang_sys::CXCursor_TemplateTypeParameter);


nit: assert_eq.

emilio · 2017-02-27T18:51:47Z

src/ir/context.rs

+    /// Get the named type defined at the given cursor location, if we've
+    /// already added one.
+    pub fn get_named_type(&self, definition: &clang::Cursor) -> Option<ItemId> {
+        assert!(definition.kind() == clang_sys::CXCursor_TemplateTypeParameter);


assert_eq. Was going to suggest moving to a debug_assert, given it's an FFI call, but I guess it's ok with #545.

emilio · 2017-02-27T18:58:46Z

src/ir/named.rs

@@ -327,10 +327,11 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen>  {
                    &TypeKind::TemplateInstantiation(ref inst) => {
                        let decl = ctx.resolve_type(inst.template_definition());
                        let args = inst.template_arguments();
+                        // Although template definitions should always have
+                        // template parameters, there is a single exception:
+                        // opaque templates. Hence the unwrap_or.


Maybe assert that it's opaque?

This ends up being a bit gross to assert because of typedefs having different names than the underlying type, needing to see through resolved type references, and the fact that either of the template instantiation or the template definition can be opaque.

Unless you feel very strongly about it, I'm going to skip this assert for now.

emilio · 2017-02-27T19:05:58Z

src/ir/context.rs

@@ -319,7 +319,8 @@ impl<'ctx> BindgenContext<'ctx> {
               item,
               definition);

-        assert!(item.is_named());
+        assert!(*item.as_type().unwrap().kind() == TypeKind::Named,


nit: item.expect_type().kind().is_named()?

If the PartialEq commit is due to this assert, I'd rather remove it and add a convenience method to TypeKind, or use the matches crate.

If the PartialEq commit is due to this assert, I'd rather remove it and add a convenience method to TypeKind

SGTM

emilio · 2017-02-27T19:07:48Z

src/ir/template.rs

@@ -34,6 +34,22 @@ use super::derive::CanDeriveCopy;
 use super::item::Item;
 use super::traversal::{EdgeKind, Trace, Tracer};

+/// A trait for things which may or may not be a named template type parameter.
+pub trait AsNamed {


I'd probably rename this to AsNamedType, what do you think?

I kind of want to do a renaming pass "named" -> "type parameter" everywhere as a follow up. What do you think of that instead? (So this would become AsTypeParameter or AsTypeParam).

emilio · 2017-02-27T19:08:21Z

tests/expectations/lib.rs

@@ -0,0 +1,3 @@
+#![allow(dead_code)]
+#![allow(non_camel_case_types)]


This fixes an issue somewhere right?

Partially. I still think we want to insert this into all the bindings all the time.

Also, for some reason I still see the warnings only some of the time in the expectations tests, and I can't figure out why... This is definitely an improvement, however.

emilio · 2017-02-27T19:11:00Z

src/ir/named.rs

@@ -257,7 +257,7 @@ pub struct UsedTemplateParameters<'ctx, 'gen>
    where 'gen: 'ctx
 {
    ctx: &'ctx BindgenContext<'gen>,
-    used: HashMap<ItemId, ItemSet>,
+    used: HashMap<ItemId, Option<ItemSet>>,


Perhaps a comment here saying that the Option here is just for moving out of the hashmap and in again would be helpful.

emilio · 2017-02-27T19:11:52Z

src/ir/named.rs

-        let original_len = self.used[&id].len();
+        // Invariant: all hash map entries' values are `Some` upon entering and
+        // exiting this method.
+        debug_assert!(self.used.values().all(|v| v.is_some()));


This can get quite hot I guess, could we maybe running it only on testing or something?

(Maybe it's not such an issue)

It will only run in debug builds, which I don't think anyone is using outside hacking on bindgen itself, so I think we're fine.

Also, these sets are very very rarely even as big as 10 elements.

It will only run in debug builds, which I don't think anyone is using outside hacking on bindgen itself, so I think we're fine.

You forget everyone doing a debug build with build-time bindgen, unfortunately :(

Ugh, you're right :(

I think we should probably define our own testing_assert!(...) macro that only runs under tests and replace all of our debug asserts with that.

Follow up?

That sounds good.

fitzgen · 2017-02-27T19:23:57Z

So it looks like we are spending about the same amount of time on bindings generation. If I keep regenerating bindings, the numbers fluctuate a bit, but are about the same in either case. Sometimes master is faster than with this PR, sometimes the other way around. Doesn't look like this PR has much of an impact on performance either way.

emilio · 2017-02-27T19:25:29Z

So it looks like we are spending about the same amount of time on bindings generation. If I keep regenerating bindings, the numbers fluctuate a bit, but are about the same in either case. Sometimes master is faster than with this PR, sometimes the other way around. Doesn't look like this PR has much of an impact on performance either way.

That sounds fine! I'd run it on stylo when I have the chance :)

fitzgen · 2017-02-27T19:28:18Z

r? @emilio :)

This is ready for a real review now (assumign that you can use it to generate stylo bindings successfully)

fitzgen · 2017-02-27T21:22:34Z

My main concern is that the actual unused template parameter can be pretty useful when writing abstractions on top of the bindgen-generated code.

This is a bit unfortunate. However, it is also the kind of thing where leveraging these templates on the Rust side already requires a bit of wrapping in a Rust-y API, and so a little bit more of that wrapping (adding a phantom type and whatever methods) doesn't seem like the end of the world. The trade off we're getting is more correctness, which almost always seems like a good trade off.

It seems (from the reviewed code experience, which might not be so much) that switching from this code generation to the alternative (what we have now, template params + PhantomData) shouldn't be extremely hard (of course, it'd bring back the issues we have with template aliases). Is that right?

We could add an annotation that is like "please consider all my template parameters used" and it should be pretty easy. Unless it is crucial for Stylo, I'd like to do such things as a follow up.

fitzgen · 2017-02-27T21:43:29Z

We could add an annotation that is like "please consider all my template parameters used" and it should be pretty easy. Unless it is crucial for Stylo, I'd like to do such things as a follow up.

And we could deny this annotation/flag/option on type aliases, which is where the correctness issues come into play.

bors-servo · 2017-03-01T17:50:55Z

☔ The latest upstream changes (presumably #549) made this pull request unmergeable. Please resolve the merge conflicts.

fitzgen · 2017-03-01T22:34:35Z

For posterity, here is the creduced version of the failing stylo bindings:

template <typename> class a;
template <typename b, typename... c> class a<b(c...)> { a(const a &); };
template <typename b, typename... c> a<b(c...)>::a(const a &) {}

I really hope that this isn't creduce "kindly" figuring out that we don't support anonymous template parameters again, and skipping past the real issue I was trying to pin down... Although I do have a branch with a fix for the anonymous type parameters that I should be able to rebase pretty quickly.

fitzgen · 2017-03-03T17:13:02Z

Small update:

I fixed that test case, but am creduceing another failure when generating Stylo bindings.

I think the fully pre-processed Stylo bindings are even bigger than SpiderMonkeys, and creduce is taking almost 24h to come up with reductions on Stylo bindings :-/

I guess the bright side is that it's good to get more edge cases into our test suite so we won't have to do this as often in the future!

fitzgen · 2017-03-06T17:43:06Z

Next stylo-derived creduced test case:

template <typename a> struct b {
  template <typename> using c = typename a::d;
  template <typename e> using f = b<c<e>>;
  f<int> g;
};

fitzgen · 2017-03-06T19:18:49Z

Grr, we're generating infinitely sized types for the most recent creduced test case.

fitzgen · 2017-03-06T21:48:03Z

@emilio OK, this should be ready for final review now.

Fixing that last test case was pretty nasty. We can't do anything sane for associated types other than recognize them and treat them as opaque, but of course clang doesn't expose the type, so we have to do some nasty regex matching on the type's spelling :(

fitzgen · 2017-03-06T22:20:46Z

CI is green :)

emilio · 2017-02-28T19:16:23Z

src/ir/ty.rs

+                                //
+                                // https://github.com/jamesmunns/teensy3-rs/issues/9
+                                if !ty.spelling().is_empty() {
+                                    warn!("invalid type {:?}", ty);


This conditional can go away since both branches are equal.

emilio · 2017-03-07T17:52:47Z

src/ir/template.rs

+                    .map(|t| Item::from_ty_or_ref(t, t.declaration(), None, ctx))
+                    .collect()
+            })
+            .unwrap_or(vec![]);


nit: map_or?

emilio · 2017-03-07T17:55:04Z

src/clang.rs

+    /// };
+    /// ```
+    pub fn is_associated_type(&self) -> bool {
+        // This is terrible :(


emilio · 2017-03-07T18:02:44Z

r=me. The regex thing makes me a bit uneasy, but not that I have a better solution, so... Please squash everything or remove the "Fix bad rebase" commit before landing :)

* Find each item's used template parameters when we begin the codegen phase * Add TemplateDeclaration::used_template_params() This method is available during the codegen phase, and uses the information gleaned by the `ir::named::UsedTemplateParameters` analysis. * Remove Item::{applicable_template_args,signature_contains_named_type} They are replaced by the template parameter usage analysis and TemplateDeclaration::used_template_params. * Parse and de-duplicate named template type parameters * Do not attempt to determine template parameter usage when not recursively whitelisting * Add a proper TemplateInstantiation type This makes it so that CompInfo is always either a compound type definition, or a template compound type definition, never an instantiation of a template. It also pulls out TypeKind::TemplateInstantiation(<inline stuff>) to a proper ir::TemplateInstantiation type, and TypeKind::TemplateInstantiation just wraps ir::TemplateInstantiation into TypeKind. * Allow template definitions to lack template parameters because of opaque template definitions * Detect and ignore cycles deriving Copy/Debug and whether a type has a vtable * Bail out early in the face of partial template specialization We don't support it, and shouldn't continue trying to parse a type from this cursor. * Do not consider inner type's parameter usage as our own parameter usage * Do not require a parent_id for template instantiations It is not necessary, and in fact was preventing us from creating template instantiations in some places, resulting in such nonsense as a generic template definition as a base for another type. * Only join if this is NOT a named template type or a template instantiation Otherwise, we'll always consider all of a template instantiation's arguments as used, when they should only be considered used if the template definition uses that template parameter. * Consider function return and parameter types as used Although we should not follow class method edges because we cannot create new monomorphizations of methods, code can create aliases of function pointers whose return or parameter types are template parameters, and those template parameters should be considered used. * Add the AsNamed trait for things which might be a named template type This sees through ResolvedTypeReferences to get at the final named type and its canonical item id. By using this in the named template parameter usage analysis, we ensure we don't have bugs where there are ResolvedTypeReferences in the usage sets rather than the canonical named item id, which could cause template parameters to be ignored accidentally. * Do not consider an inner var's template parameter usage as our own * Make the expectations' tests less noisy * Use opaque blobs for unknown template definition types When we don't know how to generate a Rust type from a template definition (eg because it uses non-type template parameters), then we should fall back to using the instantiation's layout to generate the opaque blob. * Implement CanDeriveDebug for TemplateInstantiation We need the template instantiation's layout to determine if we can derive debug for it when the instantiation's template definition has non-type parameters. * Stop thrashing malloc when unioning ItemSets in UsedTemplateParameters Previously, we were cloning an ItemSet, which requires a malloc for non-empty sets, when taking its union with our current id's set. Now, instead of doing that, we wrap each ItemSet in an Option, and take the set out of the hash map when modifying it. This allows us to side-step the borrow checker and HashMap's lack of an analog to `slice::split_at_mut` and mutate what is logically a value in the hash map while also using immutable references of values that are physically in the hash map. * Add some tests explicitly about template parameter usage * Updated test expectations now that we are inferring template parameter usage * Reinstate the layout tests for template instantiations * Generate opaque blobs for uses of partially specialized templates This adds `TypeKind::Opaque` which signifies that we do not understand anything about the given type and that we should just generate an opaque blob based on the type's layout. It explicitly uses the opaque type kind for partially specialized templates. * Add note about None vs Some([]) in TemplateDeclaration * Do not rely on TypeKind implementing PartialEq * Prefer assert_eq!(lhs, rhs) to assert!(lhs == rhs) * Expand some comments for ir::named::UsedTemplateParameters * Expand Item::is_opaque to consider TypeKind::Opaque * Use opaque types instead of panicking Use opaque types as our last resort when resolving type references after we have collected unresolved type references instead of panicking. * Find template definitions that don't want to be found * Recognize associated template types and make them opaque

fitzgen · 2017-03-07T19:06:26Z

Fixed final nitpicks and squashed. Thanks for the review!

@bors-servo r=emilio

bors-servo · 2017-03-07T19:06:27Z

📌 Commit 1c4332a has been approved by emilio

bors-servo · 2017-03-07T19:06:30Z

⌛ Testing commit 1c4332a with merge dd1dc9e...

@emilio

All the template parameters!! The major changes in this PR are: * De-duplication of named template type parameters (this is probably the biggest part of the PR, and nastiest because it is the part that deals with libclang) * Removing the `signature_contains_named_type` stuff, and enabling the more sound template type parameter usage analysis that has been landing in bits here and there. * **LOTS** of new tests for template type parameter usage @emilio: can you also test this on the stylo bindings? I tested on the SpiderMonkey bindings and found a bug and fixed a bug related to instantiations of partially specialized templates. I'd like to make sure that there aren't any latent bugs uncovered in Stylo. This is NOT ready to merge quite yet, but is ready for some more eyeballs that are not mine. Still TODO: * [x] Rebase so that these changes will merge cleanly -- I'll get on this ASAP * [x] Time SpiderMonkey bindings generation with and without these changes to see what the overhead of the new analysis is (if any!) on Large and Real World bindings * [x] Test theses changes on Stylo (thanks @emilio!) * [ ] (optional) and time Stylo bindings generation with and without these changes as well (only if you want to, @emilio) Thanks!

bors-servo · 2017-03-07T19:28:32Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing dd1dc9e to master...

emilio · 2017-03-07T21:23:04Z

Wohoo!

highfive added the S-awaiting-review label Feb 27, 2017

fitzgen force-pushed the cleanups-without-anon-types branch from 1b71e60 to d702d31 Compare February 27, 2017 18:43

emilio reviewed Feb 27, 2017

View reviewed changes

emilio mentioned this pull request Feb 27, 2017

clang: Avoid a hot FFI call for getting the cursor kind. #545

Merged

emilio reviewed Feb 27, 2017

View reviewed changes

highfive assigned emilio Feb 27, 2017

fitzgen force-pushed the cleanups-without-anon-types branch from d702d31 to 9b8f6e3 Compare February 27, 2017 20:14

fitzgen force-pushed the cleanups-without-anon-types branch from 9b8f6e3 to 5a21b43 Compare February 28, 2017 00:43

highfive added the S-needs-rebase label Mar 1, 2017

This was referenced Mar 4, 2017

Panic with C++ template templates #555

Closed

bindgen fails with panic: "C'mon: Continue" #388

Closed

fitzgen force-pushed the cleanups-without-anon-types branch from a523b9a to 4b75a19 Compare March 6, 2017 19:07

emilio reviewed Mar 7, 2017

View reviewed changes

fitzgen force-pushed the cleanups-without-anon-types branch from 85f4447 to 1c4332a Compare March 7, 2017 19:05

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Mar 7, 2017

This was referenced Mar 7, 2017

Generating invalid code for aliases that do not use their template parameters #505

Closed

Missing template argument for inner type typedef. #422

Closed

bors-servo merged commit 1c4332a into rust-lang:master Mar 7, 2017

highfive removed the S-awaiting-merge label Mar 7, 2017

bors-servo mentioned this pull request Mar 7, 2017

objc: Implement class methods #558

Merged

This was referenced Mar 20, 2017

Ignore builtin template when parsing #594

Closed

Generating bindings for headers using STL types #598

Closed

		@@ -0,0 +1,3 @@
		#![allow(dead_code)]
		#![allow(non_camel_case_types)]

All the template parameters!! #544

All the template parameters!! #544

Conversation

fitzgen commented Feb 27, 2017 • edited Loading

highfive commented Feb 27, 2017

fitzgen commented Feb 27, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Feb 27, 2017

fitzgen commented Feb 27, 2017

fitzgen commented Feb 27, 2017

Timing SpiderMonkey Bindings Generation

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Feb 27, 2017

emilio commented Feb 27, 2017

fitzgen commented Feb 27, 2017

fitzgen commented Feb 27, 2017

fitzgen commented Feb 27, 2017

bors-servo commented Mar 1, 2017

fitzgen commented Mar 1, 2017

fitzgen commented Mar 3, 2017

fitzgen commented Mar 6, 2017

fitzgen commented Mar 6, 2017

fitzgen commented Mar 6, 2017

fitzgen commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Mar 7, 2017

fitzgen commented Mar 7, 2017

bors-servo commented Mar 7, 2017

bors-servo commented Mar 7, 2017

bors-servo commented Mar 7, 2017

emilio commented Mar 7, 2017

fitzgen commented Feb 27, 2017 •

edited

Loading