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 big-int #198

Merged
merged 10 commits into from
Apr 19, 2023
Merged

Add big-int #198

merged 10 commits into from
Apr 19, 2023

Conversation

puripuri2100
Copy link
Collaborator

No description provided.

src/big-int.satyg Show resolved Hide resolved
val sub : t -> t -> t
val mul : t -> t -> t
val div : t -> t -> t
val mod- : t -> t -> t
Copy link
Collaborator

Choose a reason for hiding this comment

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

Int.satyg の方に mod- がないのか…… でもこれはあった方がいいと思うので残しときましょう(後で Int の方に mod- を追加すべき案件)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(これは今回のPRで何かする必要はないです)

src/big-int.satyg Show resolved Hide resolved
src/big-int.satyg Show resolved Hide resolved
src/big-int.satyg Show resolved Hide resolved
val sub : t -> t -> t
val mul : t -> t -> t
val div : t -> t -> t
val mod- : t -> t -> t
Copy link
Collaborator

Choose a reason for hiding this comment

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

(これは今回のPRで何かする必要はないです)

src/big-int.satyg Outdated Show resolved Hide resolved


% 繰り上がり処理を行う
let carry-and-fix (is-plus, lst) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

あーなるほど、これって数を10進表記にした時の各桁の数字をリストとして表してるんですね。
各要素をN bitの符号なし整数として表現した方が、計算速度が上がるのとビット演算が実装しやすくなるのとで好ましいと思います。2^N進数として表記した時の各桁の数字をリストにする感じになるので実装の基本的な方針は代わらないはず。OCamlの整数型は63bitだったはずなのでキャリーとかの計算しやすさを考えるとN=62にするのがよさそう。

Copy link
Collaborator

Choose a reason for hiding this comment

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

ただ今ちゃんと動いてる時点でめちゃくちゃ偉いし、あまり一つのコードレビューを長くしたくないので。一旦このままの実装でmergeしてしまってもいいかなと思います。この実装でビット演算を実装するのはキツいと思うのでビット演算はなしで大丈夫です!

src/big-int.satyg Show resolved Hide resolved
__test__/satysrc/generic.saty Show resolved Hide resolved
src/big-int.satyg Outdated Show resolved Hide resolved
src/big-int.satyg Show resolved Hide resolved
src/big-int.satyg Show resolved Hide resolved
| [] -> str
| i::is -> sub is ((Int.to-string i)^str)
in
let (b, lst) = t in
Copy link
Collaborator

Choose a reason for hiding this comment

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

(あまり重要でない点) bis-positive のような名前をつけてくれるとコードが読みやすくなって嬉しいです。

src/big-int.satyg Show resolved Hide resolved
src/big-int.satyg Show resolved Hide resolved
src/big-int.satyg Show resolved Hide resolved
| x::y::zs -> (
if Int.(x >= 10) then
let k = x / 10 in
let new-x = x - (10 * k) in
Copy link
Collaborator

Choose a reason for hiding this comment

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

細かい点ですがここは x mod 10 と書いた方が簡潔ですね(参考:https://github.com/nyuichi/satysfi-base/blob/master/src/int.satyg#L77)

Comment on lines 112 to 116
let (is-ok, new-lst) =
lst
|> sub1 []
|> sub2
in
Copy link
Collaborator

Choose a reason for hiding this comment

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

デッドコードっぽい?

@puripuri2100
Copy link
Collaborator Author

かなり遅くなりましたが、指摘点修正しました

Copy link
Collaborator

@zeptometer zeptometer left a comment

Choose a reason for hiding this comment

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

マージしちゃってよいと思います、お疲れさまでした 👍

@zeptometer zeptometer merged commit d63528a into nyuichi:master Apr 19, 2023
@puripuri2100
Copy link
Collaborator Author

ありがとうございます🙏

This was referenced Sep 24, 2023
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.

None yet

2 participants