Change return type of TemplateParameters' methods from Option to just…

[hgallagher1993]

… Vec

Fixes #960

[emilio]

Seems like this broke a bunch of tests...

[fitzgen]

Hi @hgallagher1993, did you figure out how to run the tests locally? It looks like this PR changed a lot of the bindings generated for the test headers, which I wouldn't expect.

[hgallagher1993]

@fitzgen I had a read through the contributing.md and do mean to just run cargo test?

[hgallagher1993]

I ran cargo test and the differences seem to be pretty much just white space, in 16-byte-alignment.rs for example

fn bindgen_test_layout_rte_ipv4_tuple__bindgen_ty_1__bindgen_ty_1() {

    assert_eq!(::std::mem::size_of::<rte_ipv4_tuple__bindgen_ty_1__bindgen_ty_1>()

               , 4usize , concat ! (

               "Size of: " , stringify ! (

               rte_ipv4_tuple__bindgen_ty_1__bindgen_ty_1 ) ));

changed to

fn bindgen_test_layout_rte_ipv4_tuple__bindgen_ty_1__bindgen_ty_1() {

    assert_eq!(

        ::std::mem::size_of::<rte_ipv4_tuple__bindgen_ty_1__bindgen_ty_1>(),

        4usize,

        concat!(

            "Size of: ",

            stringify!(rte_ipv4_tuple__bindgen_ty_1__bindgen_ty_1)

        )

    );

And all the impl Clone. . . dissappeared and went in to the #[derive. . .]'s

[fitzgen]

@hgallagher1993 yes. Make sure you have the latest nightly and cargo +nightly install -f rustfmt-nightly. That should take care of the formatting issues.

The impl Clone moving to a derive is more worrying; that means that this PR changed something to do with how we decide whether to derive or implement Clone, which it probably shouldn't.

This is what src/codegen/mod.rs does before this PR:

        if item.can_derive_copy(ctx) && !item.annotations().disallow_copy() &&
            ctx.options().derive_copy
        {
            derives.push("Copy");
            if used_template_params.is_some() {
                // FIXME: This requires extra logic if you have a big array in a
                // templated struct. The reason for this is that the magic:
                //     fn clone(&self) -> Self { *self }
                // doesn't work for templates.
                //
                // It's not hard to fix though.
                derives.push("Clone");
            } else {
                needs_clone_impl = true;
            }
        }

Were we really getting Some(v) where v.is_empty() before with the tests that changed? Or did we subtly change the logic in other ways?

[fitzgen]

I guess we must have been hitting Some(v) where v.is_empty() fairly often.

[hgallagher1993]

Should this be !used_template_params.is_empty()? When the ! is put in, the impl clone's are left alone when the tests are run.

[fitzgen]

Totally. Should have caught this earlier! Good eye ;)

[fitzgen]

Ok, so next steps:

• Update your nightly + rustfmt-nightly
• Run the tests locally, this should apply a bunch of impl Clone/derive(Clone) changes. Add those changes to this commit.
• Push and leave a comment telling me to look at this PR again :)

Thanks @hgallagher1993 !

[bors-servo]

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

[hgallagher1993]

@fitzgen I've ran the tests and added in the !. . .there's still the merge conflict to go though

[fitzgen]

Great! Let me know when you've rebased (or if you need any help rebasing) and then I'll take a final look and we can merge this thing :)

[hgallagher1993]

@fitzgen The merge conflict should be gone now

[hgallagher1993]

I ran the tests locally and they all passed so I'm not really sure why it's failed here

[pepyakin]

@hgallagher1993
Hello! According to the travis build log cargo test works fine on LLVM 3.8 and fails on other versions.
If we will take a look on the failing tests we will see these:

• partial-specialization-and-inheritance.hpp
• auto.hpp

These tests are special in the sense that they doesn't have the only one expected binding!
So for example there are 3 different bindings for auto.hpp depending on LLVM version:

• tests/expectations/tests/libclang-3.8/auto.rs
• tests/expectations/tests/libclang-3.9/auto.rs
• tests/expectations/tests/libclang-4/auto.rs

You can read more here.

But unfortunatelly, this is not the only problem. If you take a look into expectations test logs you can see some failures.
The expectations tests just compile every binding and run layout in it.
Note that this isn't done when you issue cargo test.
You can run these tests with the following commands:

$ cd tests/expectations
$ cargo test

[bors-servo]

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

[hgallagher1993]

@pepyakin Ya it was only cargo test I ran and I can see the tests that failed locally now so I should have this fixed up in the next day or 2

[emilio]

Closing because this bitrotted a lot and is inactive, but thanks for the patch @hgallagher1993! :)