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

feat(core): add gcd/lcm Semigroup to segment tree #163

Merged
merged 4 commits into from
Aug 7, 2021

Conversation

hotman78
Copy link
Contributor

@hotman78 hotman78 commented Aug 6, 2021

close #152

Copy link
Collaborator

@kmyk kmyk left a comment

Choose a reason for hiding this comment

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

ありがとう。動いてそうだし基本的にはよさそう。しかし 2 点だけ更新をお願いしたい

  1. min/max とは違って gcd/lcm は型引数 (std::max<T>(a, b) とか書くときの T に相当するやつ) を取らないんだけど、これを消し忘れてるので消しておいてほしい (レビューコメント参照)。付いててもバグなく動きはするけど、後々混乱を生むと思うので
  2. おそらくローカルで動作確認をしてくれたと思います。そのときに使ったスクリプトを examples/ ディレクトリの中に追加しておいてほしい。詳細な手順は Collect examples #141 にあります

src/Jikka/Core/Language/Expr.hs Outdated Show resolved Hide resolved
src/Jikka/Core/Language/TypeCheck.hs Outdated Show resolved Hide resolved
Comment on lines +150 to +153
pattern Gcd1' t e = AppBuiltin11 Gcd1 t e

pattern Lcm1' t e = AppBuiltin11 Lcm1 t e

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pattern Gcd1' t e = AppBuiltin11 Gcd1 t e
pattern Lcm1' t e = AppBuiltin11 Lcm1 t e
pattern Gcd1' e = AppBuiltin1 Gcd1 e
pattern Lcm1' e = AppBuiltin1 Lcm1 e

ここの t は型引数 (std::max<T>(a, b) とか書くときの T と同じ) なのだけど、gcd/lcm の場合は int 専用なのでなしでよい
(ここに合わせて他の場所も更新しないとだめかも)

@kmyk
Copy link
Collaborator

kmyk commented Aug 6, 2021

(conflict についてはそのままで構いません。こちらで merge 時に直します。もちろん解消してくれてもよい)

hotman78 and others added 2 commits August 7, 2021 12:40
Co-authored-by: Kimiyuki Onaka <[email protected]>
Co-authored-by: Kimiyuki Onaka <[email protected]>
@kmyk kmyk mentioned this pull request Aug 7, 2021
kmyk added a commit that referenced this pull request Aug 7, 2021
@kmyk kmyk merged commit 68f23ca into kmyk-jikka:master Aug 7, 2021
@kmyk
Copy link
Collaborator

kmyk commented Aug 7, 2021

@hotman78 #175 経由でマージしました。ありがとうございました

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.

セグメント木や累積和に GCD や LCM を載せる
2 participants