orlandos-nl / BSON

Native Swift library for BSON (http://bsonspec.org)
https://orlandos.nl/docs/mongokitten/articles/bson
MIT License
110 stars 35 forks source link

BigEndian support #38

Closed tnakaike closed 2 years ago

tnakaike commented 6 years ago

I tried to use this package with MongoKitten on a big endian machine. At that time, I had endian problems. For the master/5.2 branch, I needed some fixes like as follows. Although I plan to make PRs to support big endian, I am wondering to know which branch should be used as the code base. Because, it seems that this package is being re-written in the develop/6.0/rewrite branch.

+#if _endian(little)
                 let double: Double = try fromBytes(storage[position..<position+8])
+#else
+                var tmp = Bytes(repeating: 0, count: 8)
+                var j: Int = 0
+                for i in (position ..< position + 8).reversed() {
+                    tmp[j] = storage[i]
+           j+=1
+       }
+                let double: Double = try fromBytes(tmp)
+#endif
Joannis commented 6 years ago

For now, definitely 5.2. 6.0 is going to evolve a lot, still.

I think the #if arch(...) is preferred over #if _endian(little) because the underscore prefixed ones are unstable APIs.

tnakaike commented 6 years ago

Sure. Thanks!

Joannis commented 6 years ago

What machine are you running Swift on? I didn't know swift ran on big endian systems.

tnakaike commented 6 years ago

Hi Joannis, Thank you for your response.

I am running Swift on an IBM mainframe 😃. https://www.ibm.com/developerworks/downloads/r/toolkitforswift/

Joannis commented 6 years ago

That must be pretty cool to work with!

tnakaike commented 6 years ago

@Joannis I am ready to submit a PR. I also would like to submit PRs to MongoKitten and CryptoKitten. One problem is that CryptoKitten has no license information. Is there any license information for CryptoKitten?

Joannis commented 6 years ago

I just added the MIT license to CryptoKitten again. Thanks for the follow-up email. I'm getting back from a long vacation now so I should be more responsive again.

tnakaike commented 6 years ago

@Joannis Thank you for your prompt action. That is very helpful.

I submitted a PR (https://github.com/OpenKitten/BSON/pull/39). Please review it.

I also submitted PRs for CryptoKitten (https://github.com/OpenKitten/CryptoKitten/pull/3) and MongoKitten (https://github.com/OpenKitten/MongoKitten/pull/162). Please review them.

For all of the PRs, I executed all of the tests on both x86 (linux) and s390x (linux). The test results on x86 were same as before my changes. Most of the tests passed on s390x though some tests failed due to the swift runtime issues for s390x. I also tested by using my Kitura application, and it worked well.

Joannis commented 6 years ago

It's hard for me to know how well this works and will keep working on s390x, of course. But they look fine 👍 I'm surprised be how few changes there are

tnakaike commented 6 years ago

These are the fixes for current test cases. I am not sure if any other issue exists. If any other issue is found on s390x, I will try to fix it.

Do you need your own tests before merging these changes?

Joannis commented 6 years ago

No, I'll merge them soon 👍 I can't run the tests myself.

tnakaike commented 6 years ago

Thank you so much!

tnakaike commented 6 years ago

Hi @Joannis ,

I have not received any response from Robert for this pull request (https://github.com/OpenKitten/BSON/pull/39). How should we proceed?

Obbut commented 6 years ago

@tnakaike I've been on a vacation for the past weeks, sorry. I will have time next friday.

tnakaike commented 6 years ago

@Obbut Thanks!