Skip to content
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

arithmetic: Add Ctz function #7

Closed
wants to merge 1 commit into from
Closed

Conversation

vilie
Copy link
Contributor

@vilie vilie commented Aug 17, 2015

Signed-off-by: Valentin Ilie valentin.ilie@intel.com

@vilie
Copy link
Contributor Author

vilie commented Aug 17, 2015

First OCaml code ever written so this needs proper review. I've tested the functionality but I am not sure if this is actually a good patch.

@@ -50,7 +52,14 @@ struct
| Abs -> Int.abs
| Not -> Int.lognot
| Clz -> fun i -> i (* TODO *)
| Ctz -> fun i -> i (* TODO *)
| Ctz -> fun i ->
if Int.to_int i == 0
Copy link
Member

Choose a reason for hiding this comment

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

== is physical equality in OCaml, you want =. Also, could say i = Int.zero instead, which is shorter.

@sunfishcode
Copy link
Member

This is cool. Is there a way we can write a test for ctz while adding this code?

@lukewagner
Copy link
Member

@sunfishcode I was hoping (myself soon or others if they go first) to add an "assert" command so that scripts could invoke a method and assert an expected result. That should make it easy to throw up unit tests for ops like this.

@jfbastien
Copy link
Member

@eminemix can you please join the WebAssembly Community Group? I can't find your name in the list. This is required for significant contributions to WebAssembly.

@vilie
Copy link
Contributor Author

vilie commented Aug 17, 2015

Sent v2.

@sunfishcode, @lukewagner
I was thinking adding test code after implementing the clz.

@jfbastien
I wall do that asap.

@rossberg
Copy link
Member

Thanks for the patch. Please let me know when you have joined the Community Group, and I will merge your PR.

Signed-off-by: Valentin Ilie <valentin.ilie@intel.com>
@vilie
Copy link
Contributor Author

vilie commented Aug 19, 2015

This PR cannot be merged due to the issue pointed out by jfbastien.

This implementation may appear as part of other PRs where the author complies with the repo guidelines.

@vilie vilie closed this Aug 19, 2015
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Mar 2, 2023
dhil added a commit to dhil/webassembly-spec that referenced this pull request Sep 21, 2023
rossberg added a commit that referenced this pull request Jul 3, 2024
rossberg pushed a commit that referenced this pull request Sep 4, 2024
Add a references section with useful links
rossberg pushed a commit that referenced this pull request Nov 6, 2024
Includes changes for bulk memory instructions and memory abbreviations.

See WebAssembly/memory64#5 and WebAssembly/memory64#6.
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.

5 participants