Add rudimentary support for `__float128` #233

fitzgen · 2016-11-10T18:43:10Z

This adds __float128 as a builtin type, and generates an opaque array
of 16 u8s to represent it in the generated bindings since Rust doesn't
have an f128 type.

Context and motivation: Somehow __float128 is getting pulled into
SpiderMonkey headers from somewhere, and the lack of __float128
support was causing bindgen to hard fail in bindings generation.

r? @emilio

fitzgen · 2016-11-10T19:03:16Z

tests/expectations/tests/float128.rs

+#[test]
+fn bindgen_test_layout_A() {
+    assert_eq!(::std::mem::size_of::<A>() , 16usize);
+    assert_eq!(::std::mem::align_of::<A>() , 16usize);


Hmmm.... It seems that this alignment check is failing on Travis CI, but it is working locally. Any ideas, @emilio?

I doubt a lot the alignment of f128 is 16. The same happens with complex types and i128 and friends. Land the test commented with a FIXME and with empty expectations, since there's no way of getting a 128-bit aligned value in rust as of right now.

Also, note that layout tests now don't run by default in regen (probably should be changed? I don't know, makes most of the use cases faster), and you need to run cargo test -p tests_expectations.

jimblandy · 2016-11-10T19:04:09Z

Would it make sense to introduce a fake Rust f128_standin newtype, so that if people come across this, they might be less mystified to find [u8; 16]?

fitzgen · 2016-11-10T19:07:08Z

Would it make sense ...

Perhaps! Depends how easy it is to add newtypes to a prelude before the generated bindings are created. I am least familiar with codegen of all of bindgen's phases so far, so I will let @emilio fill in the gaps (and point me in the right direction if we want to do this).

emilio

r=me with the constants changed, and the test commented with a FIXME.

I don't think adding a new type for this is necessary given the unlikelyhood of this...

emilio · 2016-11-10T19:17:25Z

src/clangll.rs

@@ -394,6 +394,7 @@ pub const CXType_ObjCClass: c_uint = 28;
 pub const CXType_ObjCSel: c_uint = 29;
 pub const CXType_FirstBuiltin: c_uint = 2;
 pub const CXType_LastBuiltin: c_uint = 29;
+pub const CXType_Float128: c_uint = 30;


Please move this before FirstBuiltin and make LastBuiltin 30.

emilio · 2016-11-10T19:20:12Z

tests/expectations/tests/float128.rs

+#[test]
+fn bindgen_test_layout_A() {
+    assert_eq!(::std::mem::size_of::<A>() , 16usize);
+    assert_eq!(::std::mem::align_of::<A>() , 16usize);


I doubt a lot the alignment of f128 is 16. The same happens with complex types and i128 and friends. Land the test commented with a FIXME and with empty expectations, since there's no way of getting a 128-bit aligned value in rust as of right now.

Also, note that layout tests now don't run by default in regen (probably should be changed? I don't know, makes most of the use cases faster), and you need to run cargo test -p tests_expectations.

fitzgen · 2016-11-10T20:29:45Z

@emilio fixed per the review comments.

emilio · 2016-11-10T21:50:07Z

Well, the LastBuiltin value isn't changed, but we don't use it so I don't think it matters a lot. Thanks for doing this! :)

emilio · 2016-11-10T21:50:13Z

@bors-servo r+

bors-servo · 2016-11-10T21:50:14Z

📌 Commit 5e67ceb has been approved by emilio

bors-servo · 2016-11-10T21:50:17Z

⌛ Testing commit 5e67ceb with merge 6a2ee60...

@emilio

Add rudimentary support for `__float128` This adds `__float128` as a builtin type, and generates an opaque array of 16 `u8`s to represent it in the generated bindings since Rust doesn't have an `f128` type. Context and motivation: Somehow `__float128` is getting pulled into SpiderMonkey headers from somewhere, and the lack of `__float128` support was causing bindgen to hard fail in bindings generation. r? @emilio

fitzgen · 2016-11-10T22:04:44Z

Well, the LastBuiltin value isn't changed, but we don't use it so I don't think it matters a lot. Thanks for doing this! :)

That is for loops iterating over all types, and is supposed to be the same as CXType_Float128. I suspect CXType_Float128 just got hidden by CXType_LastBuiltin when clangll.rs was originally generated, since we didn't support overlapping enum variants back than.

jeanphilippeD · 2016-11-10T22:19:43Z

tests/headers/float128.hpp

+//
+// struct A {
+//     __float128 f;
+// };


Would it make sense to have a global constant __float128 gloablaFloat to test it a bit.
This does not generate layout tests, I think.

That struct would generate layout tests, but would fail them.

Oh I see what you mean, yeah that'd be neat.

Good idea, thanks.

emilio · 2016-11-10T22:22:28Z

@bors-servo r-

Could you do that @fitzgen? Should be a minute. Also, maybe using two u64 instead of multiple u16? not a big deal, but would be closer to the real alignment.

emilio · 2016-11-10T22:22:39Z

@bors-servo delegate+

bors-servo · 2016-11-10T22:22:40Z

✌️ @fitzgen can now approve this pull request

fitzgen · 2016-11-10T23:06:19Z

Also, maybe using two u64 instead of multiple u16? not a big deal, but would be closer to the real alignment.

I originally had it like this, but decided it wasn't actually any better.

fitzgen · 2016-11-10T23:08:42Z

@bors-servo r=emilio

bors-servo · 2016-11-10T23:08:43Z

📌 Commit 5e67ceb has been approved by emilio

emilio · 2016-11-11T12:29:15Z

Bors seems dead, let's try again.

emilio · 2016-11-11T12:29:22Z

@bors-servo r+

bors-servo · 2016-11-11T12:29:22Z

📌 Commit db8dc9a has been approved by emilio

bors-servo · 2016-11-11T12:29:23Z

⌛ Testing commit db8dc9a with merge e8096ec...

@emilio

Add rudimentary support for `__float128` This adds `__float128` as a builtin type, and generates an opaque array of 16 `u8`s to represent it in the generated bindings since Rust doesn't have an `f128` type. Context and motivation: Somehow `__float128` is getting pulled into SpiderMonkey headers from somewhere, and the lack of `__float128` support was causing bindgen to hard fail in bindings generation. r? @emilio

bors-servo · 2016-11-11T13:05:40Z

💔 Test failed - status-travis

fitzgen · 2016-11-11T17:37:40Z

Looks like CXType_Float128 is llvm 3.9 only. I'll fix and re-push in a few minutes.

fitzgen · 2016-11-11T18:03:52Z

Hmm... So this still builds fine when --features llvm_stable because we just won't ever get a Clang type of kind CXType_Float128 so we have some branches that will never be reached in that case. However, it means that we get (I presume) a CXType_Unexposed or something for the __float128 and the best we can do for it is generate a c_int.

Ideally, we would be able to run certain tests only if we are not --features llvm_stable, but we don't have that infrastructure right now. Nor is adding it economical: we are moving towards the removal of that feature.

Therefore, I'm going to comment the whole test out, and add a FIXME comment for when we remove the llvm_stable feature.

This adds `__float128` as a builtin type, and generates an opaque array of 16 `u8`s to represent it in the generated bindings since Rust doesn't have an `f128` type. Context and motivation: Somehow `__float128` is getting pulled into SpiderMonkey headers from somewhere, and the lack of `__float128` support was causing bindgen to hard fail in bindings generation.

fitzgen · 2016-11-11T18:10:48Z

@bors-servo r=emilio

bors-servo · 2016-11-11T18:10:49Z

📌 Commit 53f7a50 has been approved by emilio

bors-servo · 2016-11-11T19:02:14Z

⌛ Testing commit 53f7a50 with merge c2e7f46...

@emilio

Add rudimentary support for `__float128` This adds `__float128` as a builtin type, and generates an opaque array of 16 `u8`s to represent it in the generated bindings since Rust doesn't have an `f128` type. Context and motivation: Somehow `__float128` is getting pulled into SpiderMonkey headers from somewhere, and the lack of `__float128` support was causing bindgen to hard fail in bindings generation. r? @emilio

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

☀️ Test successful - status-travis

highfive assigned emilio Nov 10, 2016

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

fitzgen commented Nov 10, 2016

View reviewed changes

emilio approved these changes Nov 10, 2016

View reviewed changes

fitzgen force-pushed the float128 branch from 7d26b9d to 5e67ceb Compare November 10, 2016 20:29

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 10, 2016

jeanphilippeD reviewed Nov 10, 2016

View reviewed changes

fitzgen force-pushed the float128 branch from 5e67ceb to db8dc9a Compare November 10, 2016 23:09

highfive added S-awaiting-review and removed S-awaiting-merge labels Nov 10, 2016

emilio closed this Nov 11, 2016

emilio reopened this Nov 11, 2016

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

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

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 11, 2016

fitzgen force-pushed the float128 branch from db8dc9a to 53f7a50 Compare November 11, 2016 18:10

highfive added S-awaiting-review and removed S-tests-failed labels Nov 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 11, 2016

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

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

bors-servo mentioned this pull request Nov 11, 2016

Use __BindegenComplex for C Complex #230

Merged

fitzgen deleted the float128 branch November 11, 2016 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rudimentary support for `__float128` #233

Add rudimentary support for `__float128` #233

fitzgen commented Nov 10, 2016

fitzgen Nov 10, 2016

emilio Nov 10, 2016

jimblandy commented Nov 10, 2016

fitzgen commented Nov 10, 2016 •

edited

Loading

emilio left a comment

emilio Nov 10, 2016

emilio Nov 10, 2016

fitzgen commented Nov 10, 2016

emilio commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

fitzgen commented Nov 10, 2016

jeanphilippeD Nov 10, 2016

emilio Nov 10, 2016

emilio Nov 10, 2016

fitzgen Nov 10, 2016

emilio commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

fitzgen commented Nov 10, 2016

fitzgen commented Nov 10, 2016

bors-servo commented Nov 10, 2016

emilio commented Nov 11, 2016

emilio commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

fitzgen commented Nov 11, 2016

fitzgen commented Nov 11, 2016

fitzgen commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

Add rudimentary support for __float128 #233

Add rudimentary support for __float128 #233

Conversation

fitzgen commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblandy commented Nov 10, 2016

fitzgen commented Nov 10, 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

fitzgen commented Nov 10, 2016

emilio commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

fitzgen commented Nov 10, 2016

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

emilio commented Nov 10, 2016

emilio commented Nov 10, 2016

bors-servo commented Nov 10, 2016

fitzgen commented Nov 10, 2016

fitzgen commented Nov 10, 2016

bors-servo commented Nov 10, 2016

emilio commented Nov 11, 2016

emilio commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

fitzgen commented Nov 11, 2016

fitzgen commented Nov 11, 2016

fitzgen commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

Add rudimentary support for `__float128` #233

Add rudimentary support for `__float128` #233

fitzgen commented Nov 10, 2016 •

edited

Loading