{"action":"created","issue":{"url":"https://api.github.com/repos/atom/superstring/issues/7","repository_url":"https://api.github.com/repos/atom/superstring","labels_url":"https://api.github.com/repos/atom/superstring/issues/7/labels{/name}","comments_url":"https://api.github.com/repos/atom/superstring/issues/7/comments","events_url":"https://api.github.com/repos/atom/superstring/issues/7/events","html_url":"https://github.com/atom/superstring/pull/7","id":200897432,"number":7,"title":"Adds asm.js support","user":{"login":"arcanis","id":1037931,"avatar_url":"https://avatars.githubusercontent.com/u/1037931?v=3","gravatar_id":"","url":"https://api.github.com/users/arcanis","html_url":"https://github.com/arcanis","followers_url":"https://api.github.com/users/arcanis/followers","following_url":"https://api.github.com/users/arcanis/following{/other_user}","gists_url":"https://api.github.com/users/arcanis/gists{/gist_id}","starred_url":"https://api.github.com/users/arcanis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arcanis/subscriptions","organizations_url":"https://api.github.com/users/arcanis/orgs","repos_url":"https://api.github.com/users/arcanis/repos","events_url":"https://api.github.com/users/arcanis/events{/privacy}","received_events_url":"https://api.github.com/users/arcanis/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2017-01-15T21:55:08Z","updated_at":"2017-01-16T23:49:15Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/atom/superstring/pulls/7","html_url":"https://github.com/atom/superstring/pull/7","diff_url":"https://github.com/atom/superstring/pull/7.diff","patch_url":"https://github.com/atom/superstring/pull/7.patch"},"body":"Here and back again 😃 \r\n\r\nThis is a followup to https://github.com/atom/text-buffer/issues/183 and https://github.com/atom-archive/buffer-offset-index/pull/2\r\n\r\nThis PR adds asm.js support to this library through embind (instead of nbind). I've tried to keep changes as minimal as possible in the core. Everything tested is supported.\r\n\r\n**Possibly breaking changes:**\r\n\r\n- https://github.com/atom/superstring/compare/master...arcanis:master#diff-959292f9667b4cf4919bbae193a31523L57\r\n\r\n  (returning sometimes an array and sometimes an object was causing unnecessary complications)\r\n\r\n- https://github.com/atom/superstring/compare/master...arcanis:master#diff-e5c4747c80e4a1872cde8e24121784c0L9\r\n\r\n  (inheriting instead of typedefing allows me to write more stable template overloads)\r\n\r\n- https://github.com/atom/superstring/compare/master...arcanis:master#diff-ae762b481da801d5c00c8c9e5890f6e9L148\r\n\r\n  (Buffer aren't supported under browser environments)\r\n\r\n... and of course the `delete()` method (which has been polyfilled as a noop inside the v8 bindings).\r\n\r\nAll the tests are currently passing, ~~except for `Patch.compose` that I have not yet been able to implement. Even if it is technically supported, the embind library has safety checks that cannot be disabled (but should, cf https://github.com/kripken/emscripten/issues/4863) that make it hard to support vectors of objects~~.\r\n\r\nWDYT?"},"comment":{"url":"https://api.github.com/repos/atom/superstring/issues/comments/272987426","html_url":"https://github.com/atom/superstring/pull/7#issuecomment-272987426","issue_url":"https://api.github.com/repos/atom/superstring/issues/7","id":272987426,"user":{"login":"arcanis","id":1037931,"avatar_url":"https://avatars.githubusercontent.com/u/1037931?v=3","gravatar_id":"","url":"https://api.github.com/users/arcanis","html_url":"https://github.com/arcanis","followers_url":"https://api.github.com/users/arcanis/followers","following_url":"https://api.github.com/users/arcanis/following{/other_user}","gists_url":"https://api.github.com/users/arcanis/gists{/gist_id}","starred_url":"https://api.github.com/users/arcanis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arcanis/subscriptions","organizations_url":"https://api.github.com/users/arcanis/orgs","repos_url":"https://api.github.com/users/arcanis/repos","events_url":"https://api.github.com/users/arcanis/events{/privacy}","received_events_url":"https://api.github.com/users/arcanis/received_events","type":"User","site_admin":false},"created_at":"2017-01-16T23:49:15Z","updated_at":"2017-01-16T23:49:15Z","body":"- The `em_wrap_type` template associates a \"local type\" (the type that is used on the C++ side) with a \"wire type\" (the type that is used on the JS side). Its `receive` method takes a Wire Type and use it to build a Local Type, and the `transmit` method do the exact opposite, taking a Local Type and 'serializing' it into a Wire Type (for example, vectors are serialized into JS arrays). There's a template specialization for each significant type, and more specializations might be added in the future (for example, if you decide to use `std::set` in the future, a new entry will be added).\r\n\r\n  The `em_wrap_fn` template takes a member function as parameter, and automatically apply the `em_wrap_type<>::receive` method on each parameter, and the `em_wrap_type<>::transmit` method on the returned value. Basically, it unserialize each argument and return a serialized value to the JS side. Those templates shouldn't have to be modified, their implementation is complete.\r\n\r\n  The `WRAP` macros are just shorthands to call the templates without specifying redundant informations (for example, the `em_wrap_fn` template requires the function pointer but also its type, which can be deduced with the `decltype` keyword).\r\n\r\n  This micro-framework is useful to support every type that isn't natively supported by embind. For example, the `flat_set` type needs to be casted to a javascript `Set`, but that obviously isn't supported by embind. Same for `std::vector`, `std::unordered_map`, etc.\r\n\r\n  - Not really. The embind library does a lot of magic for sure, but some is missing (sometimes because embind cannot make assumptions about the userland code) and has to be implemented on the wrapper itself.\r\n\r\n  - Hm, they're really different. After my initial PR I went on and used nbind to port https://github.com/facebook/yoga, and it worked pretty well. However, there's still a couple issues that are still problematic (but the maintainer is usually really quick at fixing them, which is great). All things considered, I think that nbind is the right move *generally speaking* (a larger community should support it, test it, improve it), but if you already have V8 bindings then maybe embind is the easiest way to add new ones. The API is full C++ and doesn't use macros, which make it a bit easier to figure out how to hack the engine like I do with auto-wrap & co.\r\n\r\n  - Not really. The issue isn't so much that data aren't aligned in the buffer, but rather that the buffer isn't aligned on 4 bytes. Since we're allocating a `std::vector<uint8_t>`, then the allocated buffer will not have alignment informations. I've tried allocating a `std::vector<uint8_t __attribute__((aligned(4)))>` instead, but it seems that the alignment attribute isn't used by the standard container allocators. I've read some docs, and it seems to be a problem in the C++ standard, and both Clang and GCC are affected.\r\n\r\n  - I can see the issue with the metaprogramming. The whole idea behind it is that it makes it quite easy to register new types (about ten lines), and that once implemented a binding can be used for each function, being more DRY and avoiding possible mistakes. Unfortunately, it comes with a cost, and this cost is clarity. That being said, I would be happy to review any changes you would like to make to this micro-framework if that can help you.\r\n"}}
{"actor":{"display_login":"arcanis"}}