Skip to content

Add bswap post MVP polyfills #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 27 commits into from
Closed

Add bswap post MVP polyfills #33

wants to merge 27 commits into from

Conversation

MaxGraey
Copy link
Member

@MaxGraey MaxGraey commented Feb 17, 2018

Current PR add byte swap operation polyfills
See additional Post MVP features

Think it makes sense?

TODO:

  • Tests
  • Comment descriptions

@dcodeIO
Copy link
Member

dcodeIO commented Feb 17, 2018

Polyfilling future features where possible sounds like a great idea! What do you think of putting these in their own std file?

@MaxGraey
Copy link
Member Author

Polyfilling future features where possible sounds like a great idea! What do you think of putting these in their own std file?

Sure, I put this to polyfills.ts

@dcodeIO
Copy link
Member

dcodeIO commented Feb 17, 2018

When I think about a future implementation as a builtin, this one would probably use generics instead. Do you think it's possible to implement bswap that way, by switching over sizeof<T>? Certainly it won't be portable then.

@MaxGraey
Copy link
Member Author

I think about it. But bswap<u32 | i32> and bswap<u64 | i64> has very different implementation. Can I do something like this?

function bswap<T = i32 | u32>(value: T): T {
 ....
}

function bswap<T = i64 | u64>(value: T): T {
 ....
}

@dcodeIO
Copy link
Member

dcodeIO commented Feb 17, 2018

I believe that's not possible in TypeScript, so not possible in AssemblyScript as well. But, you could check for signedness by doing an overflowing addition or something and checking the pre and post result for <0. That's not super efficient, of course, but sufficient for a polyfill I guess.

@MaxGraey
Copy link
Member Author

MaxGraey commented Feb 17, 2018

Sure. This is draft

bswap16 implementation needs rewrite

@MaxGraey
Copy link
Member Author

With if/else after optimization we get flat optimized version for every static case

@MaxGraey
Copy link
Member Author

Done!

@MaxGraey MaxGraey changed the title [WIP] Add bswap post MVP polyfills Add bswap post MVP polyfills Feb 17, 2018
/** [Polyfill] Performs the sign-agnostic reverse bytes **/
declare function bswap<T = i8 | u8 | i16 | u16 | i32 | u32 | isize | usize>(value: T): T;
/** [Polyfill] Performs the sign-agnostic reverse bytes only for last 16-bit **/
declare function bswap16<T = i8 | u8 | i16 | u16 | i32 | u32>(value: T): T;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these actually exist in the portable JS library (portable.js)? If not, you can just skip them in portable.d.ts.

Copy link
Member Author

@MaxGraey MaxGraey Feb 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll implement this in portable.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in portable, you have to decide for let's say i32 arguments only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


return <T>rotr<u64>(a | b, 32);
}
throw new TypeError("Unsupported type");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In stdlib I am trying to use assertions instead of errors, just because errors add static string data to memory while assertions do not when turned off.

Copy link
Member

@dcodeIO dcodeIO Feb 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, just an assertion without a string message, but a comment behind that shows up when using source maps, that is, unless we are implementing an existing JS interface that actually is specified to throw an error on specific occasions, of course.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@dcodeIO
Copy link
Member

dcodeIO commented Feb 18, 2018

Looking good! Let me know when you are confident with this one. And can you remove the dist files from the PR? Just trying to be sure that external stuff depending on them doesn't break unintentionally.

@MaxGraey
Copy link
Member Author

I think its ready to merge. But commits is pretty mess and unfortunately I can't squash this, so may be better create another PR with another clear history branch?

@dcodeIO
Copy link
Member

dcodeIO commented Feb 18, 2018

Can just squash-merge this :)

@MaxGraey
Copy link
Member Author

MaxGraey commented Feb 18, 2018

ok. If you can do it from your side

@MaxGraey
Copy link
Member Author

Create another PR with squashed history because squash-merge has very strong requirements and not possible in current case.

This PR can be closed

@dcodeIO dcodeIO closed this Feb 18, 2018
@MaxGraey MaxGraey deleted the add-bswap-post-mvp-polyfill branch June 12, 2018 10:00
willemneal pushed a commit to willemneal/assemblyscript that referenced this pull request Jun 12, 2019
radu-matei pushed a commit to radu-matei/assemblyscript that referenced this pull request Oct 13, 2020
* Used advice from AssemblyScript#1116

This fixes the memory leak

* Cleaned up the code

Added a comment explaining all the hard coding
radu-matei pushed a commit to radu-matei/assemblyscript that referenced this pull request Oct 13, 2020
* 'master' of github.com:jedisct1/wasa:
  Fixed the memory leak for Time.sleep (AssemblyScript#33)
  Improve Descriptor (AssemblyScript#25)
  Bump assemblyscript from 0.9.2 to 0.9.4 (AssemblyScript#27)
  Deps
  Optimizations + update dependencies (AssemblyScript#21)
  Bump @as-pect/cli from 2.7.0 to 2.8.1 (AssemblyScript#23)
  Bump assemblyscript from 0.9.1 to 0.9.2 (AssemblyScript#24)
  Fix Date.now to return milliseconds instead of microseconds (AssemblyScript#19)
  Bump assemblyscript from 0.9.0 to 0.9.1 (AssemblyScript#17)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants