Added codegen for destructors

[nikhilshagri]

Fixes #529

cc @emilio

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[emilio]

Looks pretty good! I think it requires a bit more work though, since I don't think in this state it compiles.

Also, this needs a test in bindgen-integration, you can do something like:

// Test.h
// inside class Test {...
  static bool sDestructorCalled;
  ~Test();

// lib.rs
impl Drop for Test {
    fn drop(&mut self) {
        self.destruct()
    }
}

{
    let test = Test::new();
}

assert!(Test_sDestructorCalled);

Or something like that.

[emilio]

I don't think this can possibly compile? We need to mangle this name.

[nikhilshagri]

But isn't that a valid identifier? I think Rust accepts all valid Unicode strings....

Edit: hmm...seems not, just tried using the identifier on playground.

[fitzgen]

@cynicaldevil how's this going? Anything we can do to help get this across the finish line? Questions we can answer?

[nikhilshagri]

@fitzgen I have my midterms right now, but finally got a break in between, so I can work on this now. Any suggestions on what the mangled name of the destructor should be?

[fitzgen]

Any suggestions on what the mangled name of the destructor should be?

_bindgen_destructor_<class-or-struct-name> should be OK.

[nikhilshagri]

So I'm trying to name the function as *class_name*__bindgen__destructor so that it is consistent with how the constructor is named, and I'm using the canonical_name method to get the whole string, remove the part after the tilde and append __bindgen_destructor__ to it. Is there a better way of doing this?

[fitzgen]

@cynicaldevil is the canonical_name returning a name with the ~ in it? That is not what I would expect.

The Item::name method returns a NameOptions which gives a little bit more control over the name.

I would have expected format!("_bindgen_desctructor_{}", item.canonical_name()) to Just Work, and I'm a bit surprised it isn't...

[emilio]

It isn't because the name of the destructor starts with ~. We can strip it out while parsing though.

[nikhilshagri]

How can we strip away the ~ during parsing? I can't find anywhere to do it except in codegen.

[fitzgen]

Presumably in the Function/FunctionSig parsing code in src/ir/function.rs. I'm not really convinced that's any better or worse than in codegen, however.

I suggest just getting it working and then we can look at the code in review and decide if we think there is a better way or not.

[nikhilshagri]

@fitzgen After adding countless debug! statements to understand the flow of execution, I managed to get it to work as intended :)

[fitzgen]

@cynicaldevil It looks like this is failing a few tests with libclang 3.8 on Travis CI:

 extern "C" {
-    #[link_name = "_ZN7nsSlotsD1Ev"]
+    #[link_name = "_ZN7nsSlotsD0Ev"]
     pub fn nsSlots_nsSlots_destructor(this: *mut nsSlots);
 }

Do these test expectations just need to be updated or do we need to ignore these tests with libclang <= 3.8?

If the latter, you can add // bindgen-unstable to the top of the tests.

[nikhilshagri]

I don't know why this is happening, all my expectations are already up-to-date, and I am using clang 3.9.

[gsingh93]

Bump. I'm waiting for this feature :)

[emilio]

Ok, I just looked into why tests are failing. That test has a virtual destructor, that we should avoid generating. So I'm going to pull this and land it with that fix.

[emilio]

(I mean, it's not technically wrong to generate it, but I'd rather just do the proper thing for virtual methods instead)

[emilio]

Superseded by #608, thanks for this work @cynicaldevil! :)