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

Add floating-point operations, ceil and floor #190

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

elpinal
Copy link
Contributor

@elpinal elpinal commented Sep 8, 2019

This PR adds 2 floating-point operations:

  • ceil : float -> float
  • floor : float -> float

@nyuichi
Copy link
Contributor

nyuichi commented Sep 15, 2019

@elpinal

Nice pull request 👍
Could you possibly add some more functions like the following too?

  • truncate : float -> float
  • round : float -> float
  • pow : float -> float -> float (pow x y = x^y)
  • exp : float -> float (exp x = e^x)
  • sqrt : float -> float
  • log : float -> float -> float
  • log10 : float -> float

@elpinal
Copy link
Contributor Author

elpinal commented Sep 29, 2019

@nyuichi SATySFi already has a primitive named round, with type float -> int. SATySFi's round is defined as OCaml's int_of_float, which in turn is equal to OCaml's truncate. What do you think of that?

@nyuichi
Copy link
Contributor

nyuichi commented Sep 29, 2019

@elpinal

Thanks for your feedback!

SATySFi already has a primitive named round, with type float -> int.

Aw maybe we need to use a new name other than round. One better name is float-round, I think. (cc: @gfngfn)

SATySFi's round is defined as OCaml's int_of_float, which in turn is equal to OCaml's truncate

What I want is the function called Float.trunc in OCaml's standard library, which is not equivalent to truncate in Pervasives.
I would name this float-truncate following the above convention.
To be honest, their actual names do not matter to me because I will just re-export them in satysfi-li as Float.round and Float.truncate.
So I don't have a strong opinion about this.

@gfngfn gfngfn merged commit 0a9bf53 into gfngfn:master Feb 1, 2021
@gfngfn
Copy link
Owner

gfngfn commented Feb 1, 2021

I’m so sorry for the late response… I merged only the implementation of ceil and floor for now.

Note: currently, tools/gencode/vminst.ml is automatically generated from src/frontend/bytecomp/vminstdef.yaml. Thus I updated the latter so that it generates the former one, which you had extended. We will probably put only the former under version control in the future (See: #226).

Thank you for the improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants