Skip to content

Refactoring float32 and float64 prims into a single FloatPrims functor #111

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2015
Merged

Refactoring float32 and float64 prims into a single FloatPrims functor #111

merged 1 commit into from
Oct 7, 2015

Conversation

naturaltransformation
Copy link
Contributor

This is a simple refactoring of F32 and F64 to avoid fairly significant duplication.

@naturaltransformation naturaltransformation force-pushed the master branch 2 times, most recently from 6600a56 to a9304af Compare October 5, 2015 05:32
@rossberg
Copy link
Member

rossberg commented Oct 5, 2015

Looks good, but I would prefer to factor this slightly differently:

  • Rename Float.FloatPrims to Float.Make.
  • Move the instantiations of Float.Make into separate modules f32 and f64 like existed before, which now just contain include Float.Make(struct ... end)
  • Drop float.mli.
  • Move the FLOAT signature into the Float module, and remove the Floatsig module.

This way, (1) you avoid all the extra Float.FXY abbreviations, and (2) match the idioms of the stdlib more closely.

@naturaltransformation
Copy link
Contributor Author

Good point. I am matching the stdlib idioms now, including signature names in the module.

@rossberg
Copy link
Member

rossberg commented Oct 5, 2015

LGTM modulo the TODO.

sunfishcode
sunfishcode reviewed Oct 5, 2015
View reviewed changes
ml-proto/spec/float.ml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

size wasn't in the interface before. Is there a need for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to be anymore. size was there a few commits ago when int cast was integrated in float, but I guess I just let it reappear. In any case, I removed it now. Thanks!

@naturaltransformation naturaltransformation force-pushed the master branch 2 times, most recently from 7fac340 to 0064c7a Compare October 5, 2015 16:29
sunfishcode
sunfishcode reviewed Oct 6, 2015
View reviewed changes
ml-proto/spec/float.ml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

As a minor nit, print_nan here should be named something like print_nan_significand_digits since it's just printing that one part, rather than the whole NaN.

@naturaltransformation
Copy link
Contributor Author

Ok, done with the rename.

@sunfishcode
Copy link
Member

lgtm; thanks!

sunfishcode added a commit that referenced this pull request Oct 7, 2015
@sunfishcode
Merge pull request #111 from naturaltransformation/master
45b4e65 
Refactoring float32 and float64 prims into a single FloatPrims functor
@sunfishcode sunfishcode merged commit 45b4e65 into WebAssembly:master Oct 7, 2015
alexcrichton pushed a commit to alexcrichton/spec that referenced this pull request Nov 18, 2019
@tlively
Reorder extending loads to have signed variants first (WebAssembly#111)
7c2cfdd 
This is consistent with the ordering for all other instructions that
have signed and unsigned variants. This does renumber these
instructions, but no engine or toolchain has documented support for
these instructions yet, so that should be ok.
Connicpu pushed a commit to Connicpu/wasm-spec that referenced this pull request May 11, 2020
@binji
Use 1e6 instead of 1000000 for clarity (WebAssembly#111)
64203c2
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Feb 18, 2025
@slindley
Merge pull request WebAssembly#111 from WebAssembly/further-examples
eca06cf 
Add further examples to explainer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@naturaltransformation @rossberg @sunfishcode