Use __BindegenComplex for C Complex #230

jeanphilippeD · 2016-11-08T21:18:03Z

C complex only exists for floating point types
C Complex are built in types

long double _Complex is not supported.
Long double would be an f128, runing generated binding test produces:
assertion failed: (left == right) (left: 16, right: 32)',
tests/expectations/tests/complex.rs:72
We test global long double _Complex because it does not require
layout tests.

Handle all the different way a complex can be present in
BindgenContext calling generated_bindegen_complex to indicate
that __BindgenContext will need to be added.

emilio

Whoa, looks neat!

I fear those are not the only places you need to check, so I'd rather move the flags to the context.

Apart from that, only a few comments, mostly nits.

Thanks for doing this!

emilio · 2016-11-08T21:21:30Z

src/codegen/mod.rs

@@ -1487,6 +1500,17 @@ impl ToRustTy for Type {
                    }
                }
            }
+            TypeKind::Complex(fk) => {


We should be able to call saw_complex just here, and remove the rest.

Oh, nevermind, you don't have access to the result. Maybe worth moving that to the context inside a Cell<bool>, and checking that instead? Even better would be something like Cell<SeenFlags>, where SeenFlags is a bitflags!, but for now moving saw_union and saw_complex to BindgenContext is fine, the other can be a followup if you want.

Otherwise you'll need to check for virtually every possible type, function arguments, etc...

emilio · 2016-11-08T21:22:46Z

src/codegen/mod.rs

@@ -1487,6 +1500,17 @@ impl ToRustTy for Type {
                    }
                }
            }
+            TypeKind::Complex(fk) => {
+                use ir::ty::FloatKind;
+                let float_path = match fk {


Note that there's a new option to use c_float/c_double. You should probably respect that here, reusing the same code.

emilio · 2016-11-08T21:24:00Z

src/codegen/mod.rs

+        )
+            .unwrap();
+
+        let items = vec![


nit: This is probably nicer in its own line: vec![complex_type], assuming rustfmt likes it.

emilio · 2016-11-08T21:28:27Z

src/ir/context.rs

+            CXType_Complex => {
+                let float_type = ty.elem_type()
+                    .expect("Not able to resolve complex type?");
+                let float_kind = match float_type.kind() {


probably:

let float_kind = match float_type.kind() { CXType_Float => FloatKind::Float, CXType_Double => FloatKind::Double, CXType_LongDouble => FloatKind::LongDouble, _ => panic!("Non floating-type complex?"), };

Or, you could also add a FloatKind::from_ty(Type) -> Option<FloatKind> function, though that might be overkill for this, so leaving a match is totally fine :)

emilio · 2016-11-08T21:29:52Z

src/ir/ty.rs

@@ -384,6 +385,9 @@ pub enum TypeKind {
    /// A floating point type.
    Float(FloatKind),

+    /// A complex floating point type. complex only exist for floating point.


nit: I think /// A complex floating point type. is more than enough, since it implies "floating point", but I guess the second sentence doesn't harm. Please capitalize "Complex" if you keep it :)

emilio · 2016-11-08T21:31:56Z

tests/headers/complex_global.h

@@ -0,0 +1 @@
+double _Complex globalValue;


You can probably stick that in tests/headers/complex.h if you prefer, though it's fine as is too :)

With my fix it was possible to have saw_complex called for global, but not for member variables (or vice versa).
With your suggestion that won't be the case, but might be better to keep them separate.

emilio · 2016-11-08T21:34:31Z

tests/headers/complex.h

+COMPLEX_TEST(double, Double)
+COMPLEX_TEST(float, Float)
+
+// Not supported:


Can you mention here, instead of that, something like:

// FIXME: 128-byte-aligned in some machines, which we can't support right now in Rust.

emilio · 2016-11-08T21:34:36Z

tests/headers/complex.h

+COMPLEX_TEST(float, Float)
+
+// Not supported:
+//COMPLEX_TEST(long double, LongDouble)


Please add a space after //

jeanphilippeD · 2016-11-10T23:03:46Z

Thanks a lot for all you comments, they were very helpful.
I've also added a test for global long double _Complex since it does not do layout test.

emilio

Only one thing about where to set the saw_complex flag, and two nits, otherwise looks great, thanks! 🎉

emilio · 2016-11-11T00:08:51Z

src/ir/context.rs

+                    _ => panic!("Non floating-type complex?"),
+                };
+
+                self.saw_complex.set(true);


Hmm... Probably we should only output it if we try to convert a rust type to a complex. It's likely, over all if you use whitelisting, that we end up parsing it but not generating output for it. Could you add a method to set this flag, and call it from to_rust_ty, where we generate the __BindgenComplex instead?

Note that to modify a Cell the context doesn't need to be mutable, so we can set it during codegen.

That makes a lot of sense.
Should I add a non white listed _Complex in whitelist_basic.hpp, or a new whitelist_complex.h?

Any of those options work for me :)

emilio · 2016-11-11T00:09:22Z

src/ir/context.rs

+
+    /// Whether a complex was detected anywhere
+    pub fn saw_complex(&self) -> bool {
+        return self.saw_complex.get();


nit: no need for return, self.saw_complex.get() (without the semicolon) will do.

emilio · 2016-11-11T00:10:39Z

src/codegen/mod.rs

@@ -1560,6 +1561,33 @@ fn raw_type(ctx: &BindgenContext, name: &str) -> P<ast::Ty> {
    }
 }

+fn float_kind_rust_type(ctx: &BindgenContext, fk: FloatKind) -> P<ast::Ty> {


Probably we could move this (and raw_type) to the helper module, if you wish. Otherwise it's fine :).

bors-servo · 2016-11-11T20:21:28Z

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

C complex only exists for floating point types. C Complex are built in types long double _Complex is not supported. Long double would be an f128, runing generated binding test produces: assertion failed: `(left == right)` (left: `16`, right: `32`)', tests/expectations/tests/complex.rs:72 We test global long double _Complex because it does not require layout tests. Handle all the different way a complex can be present in BindgenContext calling generated_bindegen_complex to indicate that __BindgenContext will need to be added.

jeanphilippeD · 2016-11-11T21:32:11Z

Updated the 'saw_complex' name to be a bit more specific considering it is stored in the context, but set and used in the mod.rs

emilio · 2016-11-11T23:27:15Z

Awesome! Thanks for the patch! :)

@bors-servo r+

bors-servo · 2016-11-11T23:27:17Z

📌 Commit b5d879a has been approved by emilio

bors-servo · 2016-11-11T23:27:19Z

⚡ Test exempted - status

Use __BindegenComplex for C Complex Fix #72 C complex only exists for floating point types C Complex are built in types long double _Complex is not supported. Long double would be an f128, runing generated binding test produces: assertion failed: `(left == right)` (left: `16`, right: `32`)', tests/expectations/tests/complex.rs:72 We test global long double _Complex because it does not require layout tests. Handle all the different way a complex can be present in BindgenContext calling generated_bindegen_complex to indicate that __BindgenContext will need to be added.

highfive added the S-awaiting-review label Nov 8, 2016

emilio reviewed Nov 8, 2016

View reviewed changes

emilio added S-needs-code-changes and removed S-awaiting-review labels Nov 9, 2016

jeanphilippeD force-pushed the issue_72 branch from b0bca66 to 48b0318 Compare November 10, 2016 20:58

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 10, 2016

jeanphilippeD force-pushed the issue_72 branch 2 times, most recently from 8325d71 to d69f4c7 Compare November 10, 2016 22:28

emilio reviewed Nov 11, 2016

View reviewed changes

highfive added the S-needs-rebase label Nov 11, 2016

jeanphilippeD force-pushed the issue_72 branch from d69f4c7 to 0f3f1fc Compare November 11, 2016 21:10

jeanphilippeD force-pushed the issue_72 branch from 0f3f1fc to b5d879a Compare November 11, 2016 21:17

highfive assigned emilio Nov 11, 2016

highfive removed S-awaiting-review S-needs-rebase labels Nov 11, 2016

highfive added the S-awaiting-merge label Nov 11, 2016

bors-servo merged commit b5d879a into rust-lang:master Nov 11, 2016

highfive removed the S-awaiting-merge label Nov 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use __BindegenComplex for C Complex #230

Use __BindegenComplex for C Complex #230

jeanphilippeD commented Nov 8, 2016 •

edited

Loading

emilio left a comment

emilio Nov 8, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

jeanphilippeD Nov 10, 2016

emilio Nov 8, 2016

emilio Nov 8, 2016

jeanphilippeD commented Nov 10, 2016

emilio left a comment

emilio Nov 11, 2016

jeanphilippeD Nov 11, 2016

emilio Nov 11, 2016

emilio Nov 11, 2016

emilio Nov 11, 2016

bors-servo commented Nov 11, 2016

jeanphilippeD commented Nov 11, 2016

emilio commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

Use __BindegenComplex for C Complex #230

Use __BindegenComplex for C Complex #230

Conversation

jeanphilippeD commented Nov 8, 2016 • edited Loading

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

jeanphilippeD commented Nov 10, 2016

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

bors-servo commented Nov 11, 2016

jeanphilippeD commented Nov 11, 2016

emilio commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

jeanphilippeD commented Nov 8, 2016 •

edited

Loading