Use fully disambiguated name instead of a number for layout tests (fixes #394) #829

Manishearth · 2017-07-20T06:51:18Z

These numbers cause tons of churn in the diffs for checked in bindings.

emilio

CI is red, you need to update the platform-specific tests (you can do it using the diffs from travis).

emilio · 2017-07-20T10:48:02Z

src/ir/item.rs

+    /// Create a fully disambiguated name for an item, including template
+    /// parameters if it is a type
+    pub fn full_disambiguated_name(&self, ctx: &BindgenContext) -> String {
+        let mut s = "".into();


nit: just String::new()?

emilio · 2017-07-20T10:49:04Z

src/ir/item.rs

+    /// Helper function for full_disambiguated_name
+    fn push_disambiguated_name(&self, ctx: &BindgenContext, to: &mut String, level: u8) {
+        to.push_str(&self.canonical_name(ctx));
+        match *self.kind() {


Seems like a case for either if let, or:

let ty = match *self.kind() { ItemKind::Type(ref ty) => ty, _ => return, }; // ...

emilio · 2017-07-20T10:51:29Z

src/codegen/mod.rs

@@ -768,7 +768,7 @@ impl CodeGenerator for TemplateInstantiation {
            let size = layout.size;
            let align = layout.align;

-            let name = item.canonical_name(ctx);
+            let name = item.full_disambiguated_name(ctx);
            let mut fn_name = format!("__bindgen_test_layout_{}_instantiation", name);
            let times_seen = result.overload_number(&fn_name);


I guess you can remove this overload_number stuff and so on?

Still necessary (https://github.com/servo/rust-bindgen/blob/fbb68d4b0204ae5f47df16c0f71ce34cf56837c4/tests/expectations/tests/template.rs#L248)

emilio · 2017-07-20T10:52:39Z

src/ir/item.rs

+                TypeKind::TemplateInstantiation(ref inst) => {
+                    to.push_str(&format!("_open{}_", level));
+                    for arg in inst.template_arguments() {
+                        ctx.resolve_item(*arg).push_disambiguated_name(ctx, to, level + 1);


This won't work for nested templates, afaict, because you don't handle ResolvedTypeRef, or anything like that.

fitzgen · 2017-07-20T16:41:56Z

I still think that using the C++ mangling would be best (they already solved this exact unique-ness issue), do you remember why we weren't getting the mangling for template instantiations, @emilio ? Was it just that we weren't saving the information?

Anyways, cancelling my review, since Emilio is on it.

fitzgen · 2017-07-20T22:05:22Z

Looks like the libclang-4 expectations need to be updated:

---- header_issue_769_bad_instantiation_test_hpp stdout ----
	/home/travis/build/servo/rust-bindgen/tests/headers/issue-769-bad-instantiation-test.hpp:9:19: warning: alias declarations are a C++11 extension [-Wc++11-extensions], err: false
/home/travis/build/servo/rust-bindgen/tests/headers/issue-769-bad-instantiation-test.hpp:10:25: warning: alias declarations are a C++11 extension [-Wc++11-extensions], err: false
diff expected generated
--- expected: "/home/travis/build/servo/rust-bindgen/tests/expectations/tests/libclang-4/issue-769-bad-instantiation-test.rs"
+++ generated from: "/home/travis/build/servo/rust-bindgen/tests/headers/issue-769-bad-instantiation-test.hpp"
 /* automatically generated by rust-bindgen */
 
 
 #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
 
 
 #[allow(non_snake_case, non_camel_case_types, non_upper_case_globals)]
 pub mod root {
     #[allow(unused_imports)]
     use self::super::root;
     #[repr(C)]
     #[derive(Debug, Copy, Clone)]
     pub struct Rooted<T> {
         pub member: T,
         pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell<T>>,
     }
     impl <T> Default for Rooted<T> {
         fn default() -> Self { unsafe { ::std::mem::zeroed() } }
     }
     pub type AutoValueVector_Alias = ::std::os::raw::c_int;
     #[test]
     fn __bindgen_test_layout_Rooted_open0_int_close0_instantiation() {
         assert_eq!(::std::mem::size_of::<root::Rooted<::std::os::raw::c_int>>()
                    , 4usize , concat ! (
                    "Size of template specialization: " , stringify ! (
                    root::Rooted<::std::os::raw::c_int> ) ));
         assert_eq!(::std::mem::align_of::<root::Rooted<::std::os::raw::c_int>>()
                    , 4usize , concat ! (
                    "Alignment of template specialization: " , stringify ! (
                    root::Rooted<::std::os::raw::c_int> ) ));
     }
     #[test]
-    fn  __bindgen_test_layout_Rooted_open0_AutoValueVector_Alias_close0_instantiation() {
+    fn __bindgen_test_layout_Rooted_open0_AutoValueVector_Alias_close0_instantiation() {
         assert_eq!(::std::mem::size_of::<root::Rooted<root::AutoValueVector_Alias>>()
                    , 4usize , concat ! (
                    "Size of template specialization: " , stringify ! (
                    root::Rooted<root::AutoValueVector_Alias> ) ));
         assert_eq!(::std::mem::align_of::<root::Rooted<root::AutoValueVector_Alias>>()
                    , 4usize , concat ! (
                    "Alignment of template specialization: " , stringify ! (
                    root::Rooted<root::AutoValueVector_Alias> ) ));
     }
 }

#394)

Manishearth · 2017-07-21T00:04:51Z

Passes now

emilio

r=me

emilio · 2017-07-21T09:36:05Z

@bors-servo r+

bors-servo · 2017-07-21T09:36:06Z

📌 Commit 3dedc2c has been approved by emilio

bors-servo · 2017-07-21T09:36:08Z

⌛ Testing commit 3dedc2c with merge b51ddf3...

@fitzgen

Use fully disambiguated name instead of a number for layout tests (fixes #394) These numbers cause tons of churn in the diffs for checked in bindings. r? @fitzgen

bors-servo · 2017-07-21T10:10:25Z

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

Bump to 0.28 Brings in #822 which unblocks https://bugzilla.mozilla.org/show_bug.cgi?id=1366956, and also #829 which should greatly reduce merge conflicts in checked in bindings.

highfive assigned fitzgen Jul 20, 2017

highfive added the S-awaiting-review label Jul 20, 2017

Manishearth force-pushed the disambig branch 2 times, most recently from df88db7 to fbb68d4 Compare July 20, 2017 07:26

emilio reviewed Jul 20, 2017

View reviewed changes

fitzgen assigned emilio and unassigned fitzgen Jul 20, 2017

Manishearth force-pushed the disambig branch 2 times, most recently from cbbe118 to 19b69fb Compare July 20, 2017 17:15

Use fully disambiguated name instead of a number for layout tests (fixes

3dedc2c

#394)

Manishearth force-pushed the disambig branch from 19b69fb to 3dedc2c Compare July 20, 2017 22:10

emilio approved these changes Jul 21, 2017

View reviewed changes

highfive removed the S-awaiting-review label Jul 21, 2017

highfive added the S-awaiting-merge label Jul 21, 2017

bors-servo pushed a commit that referenced this pull request Jul 21, 2017

Auto merge of #829 - servo:disambig, r=emilio

b51ddf3

Use fully disambiguated name instead of a number for layout tests (fixes #394) These numbers cause tons of churn in the diffs for checked in bindings. r? @fitzgen

bors-servo merged commit 3dedc2c into master Jul 21, 2017

highfive removed the S-awaiting-merge label Jul 21, 2017

fitzgen mentioned this pull request Jul 21, 2017

Generate properly mangled test names #723

Closed

Manishearth mentioned this pull request Jul 21, 2017

Bump to 0.28 #841

Merged

upsuper mentioned this pull request Jul 23, 2017

Layout test can contain whitespace inside the function name which causes unexpected token error #844

Closed

emilio deleted the disambig branch July 23, 2017 01:22

upsuper mentioned this pull request Aug 1, 2017

Stablize name of pointer and array #872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fully disambiguated name instead of a number for layout tests (fixes #394) #829

Use fully disambiguated name instead of a number for layout tests (fixes #394) #829

Manishearth commented Jul 20, 2017

emilio left a comment

emilio Jul 20, 2017

emilio Jul 20, 2017

emilio Jul 20, 2017

Manishearth Jul 20, 2017

emilio Jul 20, 2017

fitzgen commented Jul 20, 2017

fitzgen commented Jul 20, 2017

Manishearth commented Jul 21, 2017

emilio left a comment

emilio commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017

Use fully disambiguated name instead of a number for layout tests (fixes #394) #829

Use fully disambiguated name instead of a number for layout tests (fixes #394) #829

Conversation

Manishearth commented Jul 20, 2017

emilio left a comment

Choose a reason for hiding this comment

emilio Jul 20, 2017

Choose a reason for hiding this comment

emilio Jul 20, 2017

Choose a reason for hiding this comment

emilio Jul 20, 2017

Choose a reason for hiding this comment

Manishearth Jul 20, 2017

Choose a reason for hiding this comment

emilio Jul 20, 2017

Choose a reason for hiding this comment

fitzgen commented Jul 20, 2017

fitzgen commented Jul 20, 2017

Manishearth commented Jul 21, 2017

emilio left a comment

Choose a reason for hiding this comment

emilio commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017