Return Vec on TemplateParameters #1245

ericho · 2018-02-04T06:46:29Z

The Option returned in this trait was removed to return Vec.

Fixes #960.

ericho · 2018-02-04T07:11:28Z

src/ir/context.rs

+                        //             num_template_params,
+                        //         )
+                        //     },
+                        // )


I will remove this!

ericho · 2018-02-04T07:33:40Z

Ha! I noticed two things..

There are several failures in Travis. It seems that running the tests twice hides some errors and everything seems ok. The errors I see are not the same as Travis shows. Need to investigate more.
I didn't see the PR Change return type of TemplateParameters' methods from Option to just… #1064 that fixes the same issue as this one..

emilio

Hi, thanks a lot for doing this, it's great! Just some nits and questions :)

emilio · 2018-02-04T12:44:56Z

src/codegen/mod.rs

@@ -2972,7 +2966,7 @@ impl TryToRustTy for Type {
            TypeKind::Comp(ref info) => {
                let template_params = item.used_template_params(ctx);
                if info.has_non_type_template_params() ||
-                    (item.is_opaque(ctx, &()) && template_params.is_some())
+                    (item.is_opaque(ctx, &()) && template_params.len() > 0)


nit: !is_empty().

emilio · 2018-02-04T12:48:05Z

src/ir/item.rs

@@ -1112,18 +1112,18 @@ where
    fn self_template_params(
        &self,
        ctx: &BindgenContext,
-    ) -> Option<Vec<TypeId>> {
+    ) -> Vec<TypeId> {
        ctx.resolve_item_fallible(*self).and_then(|item| {


nit: you can use .map here, and remove the Some( below.

And actually can use map_or(vec![], |item| item.self_template_params(ctx)), which is slightly nicer :)

It's done :) the usage of map_or is nicer !

emilio · 2018-02-04T12:50:09Z

src/ir/template.rs

-                    .collect(),
-            )
-        }
+            .filter_map(|id| {


nit: Just .flat_map(|id| id.self_template_params(ctx)).collect() would do here I suspect.

I tried that but I got the following error:

--> src/ir/template.rs:144:58 | 144 | .flat_map(|id| id.self_template_params(ctx)).collect(); | ^^^^^^^ a collection of type `std::vec::Vec<std::vec::Vec<_>>` cannot be built from an iterator over elements of type `ir::context::TypeId` | = help: the trait `std::iter::FromIterator<ir::context::TypeId>` is not implemented for `std::vec::Vec<std::vec::Vec<_>>`

So, I'm not sure how to proceed here.

emilio · 2018-02-04T12:54:41Z

src/ir/context.rs

-                                )
-                            },
-                        )
+                        Some((


So this I think changes behavior in a non-obvious way: before we'd only return Some here only if there was any template parameter, now we don't... Have you checked the callers are ok with that?

If they are, you can remove the Some( and just use map instead of and_then in the line above.

If they're not, then you can save template_decl_id.num_self_template_params(self) in a local, and return None if num_template_params == 0.

I believe I fixed this, I added the num_template_params == 0 check

emilio · 2018-02-04T12:54:57Z

src/ir/context.rs

-                                )
-                            },
-                        )
+                        Some((


Same issue as above

emilio · 2018-02-04T12:57:27Z

src/codegen/mod.rs

@@ -1657,7 +1652,7 @@ impl CodeGenerator for CompInfo {
            derives.push("Copy");

            if ctx.options().rust_features().builtin_clone_impls() ||
-                used_template_params.is_some()
+                used_template_params.len() > 0


nit: !is_empty.

Also the travis failures are on rust stable, and look a lot like they're related to this line (they're generating a bunch of unnecessary clone impls).

That being said, I think this is actually fixing a bug... So let's just update the test expectations :)

:O That's nice!

Just to confirm, does that means that I should include all the modifications under tests/expectations/tests folder ?

emilio · 2018-02-04T13:02:51Z

src/ir/template.rs

-                    .collect()
-            },
-        )
+        ctx.resolve_item(id).all_template_params(ctx)


Hmm, in particular, used_template_params used to mean a different thing with the Option<..>...

It used to return Some if there was any (used or unused) template parameter. then the Vec inside contained only the ones that were actually used...

emilio · 2018-02-04T13:02:51Z

src/ir/template.rs

-                    .collect()
-            },
-        )
+        ctx.resolve_item(id).all_template_params(ctx)


Hmm, so, used_template_params used to mean a different thing with the Option<..>...

It used to return Some if there was any (used or unused) template parameter. then the Vec inside contained only the ones that were actually used...

But per the above the only use that changed behavior in our test-suite I think was actually buggy, so yeah, this is fine :)

emilio · 2018-02-04T13:04:38Z

src/codegen/mod.rs

-                    } else {
-                        Some(ps)
-                    });
+                let mut outer_params = match item.used_template_params(ctx).len() {


This should probably be just:

let used_params = item.used_template_params(ctx); let outer_params = if used_params.is_empty() { None } else { Some(used_params) };

Or something of the sort.

Yes!, that's cleaner, thanks! 😄

ericho · 2018-02-04T19:53:41Z

tests/expectations/tests/nsBaseHashtable.rs

+    #[inline]
+    pub unsafe fn new(arg1: *mut nsBaseHashtable_EntryType, arg2: *mut nsBaseHashtable) -> Self {
+        let mut __bindgen_tmp = ::std::mem::uninitialized();
+        nsBaseHashtable_LookupResult_LookupResult(&mut __bindgen_tmp, arg1, arg2);


Test building is failing due to this line, it looks weird the repeated LookupResult_LookupResult string here.

Oh I see what's going on... We're now generating constructors and such for structs that don't use any template parameter. That's bogus actually, since LookupResult is parameterized by a bunch of stuff, so that condition needs to use all_template_parameters or something of the sort.

Do you have any advice on where to start looking to solve this problem? I'm kind of ~~completely~~ lost right now.

Yes, sorry :)

So this is probably related to this check:

https://github.com/rust-lang-nursery/rust-bindgen/pull/1245/files#diff-567e947a8cc5ad306d3eff99dfb66cc9R1735

Your patch somehow made that condition change. Those are nested template structs, so it's kind of tricky, but I'd double-check with and without your changes running some of the reduced tests.

I added manually the extern definitions and now the code compiles. Is this something that should be generated automatically?

extern "C" { pub fn nsBaseHashtable_EntryPtr_EntryPtr_destructor(this: *mut nsBaseHashtable_EntryPtr); pub fn nsBaseHashtable_EntryPtr_EntryPtr(this: *mut nsBaseHashtable_EntryPtr, arg1: *mut nsBaseHashtable, arg2: *mut nsBaseHashtable_EntryType, arg3: bool); pub fn nsBaseHashtable_LookupResult_LookupResult(this: *mut nsBaseHashtable_LookupResult, arg1: *mut nsBaseHashtable_EntryType, arg2: *mut nsBaseHashtable); }

Not really, that will compile, but will not link. We can't generate them because they have no symbols, being them in a templated struct.

bors-servo · 2018-02-14T19:45:50Z

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

The Option<T> returned in this trait was removed to return Vec. Fixes rust-lang#960.

bors-servo · 2018-03-13T11:46:04Z

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

@emilio

TemplateParameters do not return Option Fixes #960. Closes #1245. I found it useful to do this incrementally, changing each method in a separate commit and ensuring the tests continue to pass unchanged. r? @emilio /cc @ericho

@emilio

TemplateParameters do not return Option Fixes #960. Closes #1245. I found it useful to do this incrementally, changing each method in a separate commit and ensuring the tests continue to pass unchanged. r? @emilio /cc @ericho

highfive added the S-awaiting-review label Feb 4, 2018

ericho commented Feb 4, 2018

View reviewed changes

src/ir/context.rs Outdated

// num_template_params,

// )

// },

// )

Copy link

Author

ericho Feb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove this!

emilio reviewed Feb 4, 2018

View reviewed changes

ericho force-pushed the ericho-fixoption branch 2 times, most recently from 2d9f48c to 6fbb0be Compare February 4, 2018 18:49

ericho commented Feb 4, 2018

View reviewed changes

highfive added the S-needs-rebase label Feb 14, 2018

emilio mentioned this pull request Feb 17, 2018

TemplateParameters' methods should just return a Vec, not an Option<Vec> #960

Closed

Return Vec on TemplateParameters

b03316a

The Option<T> returned in this trait was removed to return Vec. Fixes rust-lang#960.

ericho force-pushed the ericho-fixoption branch from 6fbb0be to b03316a Compare February 19, 2018 04:11

highfive added the S-needs-rebase label Mar 13, 2018

tamird mentioned this pull request Apr 7, 2018

TemplateParameters do not return Option #1305

Merged

emilio closed this in #1305 Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return Vec on TemplateParameters #1245

Return Vec on TemplateParameters #1245

ericho commented Feb 4, 2018

ericho Feb 4, 2018

ericho commented Feb 4, 2018

emilio left a comment

emilio Feb 4, 2018

emilio Feb 4, 2018

emilio Feb 4, 2018

ericho Feb 4, 2018

emilio Feb 4, 2018

ericho Feb 4, 2018

emilio Feb 4, 2018

ericho Feb 4, 2018

emilio Feb 4, 2018

emilio Feb 4, 2018

emilio Feb 4, 2018

emilio Feb 4, 2018

ericho Feb 4, 2018

ericho Feb 4, 2018

emilio Feb 4, 2018

emilio Feb 4, 2018

emilio Feb 4, 2018

emilio Feb 4, 2018

ericho Feb 4, 2018

ericho Feb 4, 2018

emilio Feb 4, 2018

ericho Feb 6, 2018

emilio Feb 6, 2018

ericho Feb 11, 2018

emilio Feb 12, 2018

bors-servo commented Feb 14, 2018

bors-servo commented Mar 13, 2018

Return Vec on TemplateParameters #1245

Return Vec on TemplateParameters #1245

Conversation

ericho commented Feb 4, 2018

Choose a reason for hiding this comment

ericho commented Feb 4, 2018

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

Choose a reason for hiding this comment

bors-servo commented Feb 14, 2018

bors-servo commented Mar 13, 2018