Add GCD and LCM #25
Add GCD and LCM #25
Conversation
src/number.c
Outdated
| /* BEGIN code for computing gcd of two numbers | ||
| result = GCD(result, right); */ |
There was a problem hiding this comment.
Do we need this comment?
This is only the main computation part. So I feel that this is redundant.
src/number.c
Outdated
| n1 = result; | ||
| n2 = SCM_INT_VALUE(right); | ||
|
|
||
| if (n1 < 0) n1 = - n1; | ||
| if (n2 < 0) n2 = - n2; | ||
|
|
||
| while (n1 != 0) { | ||
| tmp = n1; | ||
| n1 = n2 % n1; | ||
| n2 = tmp; | ||
| } | ||
| result = n2; |
There was a problem hiding this comment.
Could you use spaces instead of tab for indent?
test/test-number-arith.scm
Outdated
| (assert-equal? (tn) 1 (gcd 20 1)) | ||
| (assert-equal? (tn) 4 (gcd 8 20 32)) | ||
| (tn "lcm") | ||
| (assert-equal? (tn) 288 (gcd 32 -36)) |
There was a problem hiding this comment.
| (assert-equal? (tn) 288 (gcd 32 -36)) | |
| (assert-equal? (tn) 288 (lcm 32 -36)) |
src/number.c
Outdated
| if (n1 < 0) n1 = - n1; | ||
| if (n2 < 0) n2 = - n2; |
There was a problem hiding this comment.
| if (n1 < 0) n1 = - n1; | |
| if (n2 < 0) n2 = - n2; | |
| if (n1 < 0) n1 = -n1; | |
| if (n2 < 0) n2 = -n2; |
src/number.c
Outdated
| if (n1 < 0) n1 = - n1; | ||
| if (n2 < 0) n2 = - n2; |
There was a problem hiding this comment.
| if (n1 < 0) n1 = - n1; | |
| if (n2 < 0) n2 = - n2; | |
| if (n1 < 0) n1 = -n1; | |
| if (n2 < 0) n2 = -n2; |
src/number.c
Outdated
| if (n1 < 0) n1 = - n1; | ||
| if (n2 < 0) n2 = - n2; | ||
|
|
||
| big = n1 > n2 ? n1 : n2; | ||
| small = n1 < n2 ? n1 : n2; | ||
| n1 = big; | ||
|
|
||
| while (n1 % small != 0) | ||
| n1 += big; | ||
|
|
||
| result = n1; |
There was a problem hiding this comment.
How about defining static scm_int_t gcd(scm_int_t x, scm_int_t y) and using it in scm_p_gcd() and scm_p_lcm()?
|
I believe all issues are dealt with. Please tell me if there is anything else to be changed. |
src/number.c
Outdated
| SCM_EXPORT ScmObj | ||
| scm_p_lcm(ScmObj left, ScmObj right, enum ScmReductionState *state) | ||
| { | ||
| scm_int_t result, big, small, n1, n2; |
There was a problem hiding this comment.
We need to update this.
Could you enable GitHub Actions on your fork so that you can check CI results on your fork?
src/number.c
Outdated
| case SCM_REDUCE_1: | ||
| ENSURE_INT(right); | ||
| result *= SCM_INT_VALUE(right) / gcd(result, SCM_INT_VALUE(right)); | ||
| /* Fall through. */ |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Could you update the PR description before we merge this?
We'll use the PR description as commit message.
src/number.c
Outdated
| default: | ||
| SCM_NOTREACHED; | ||
| } | ||
| if (result < 0) result = -result; |
There was a problem hiding this comment.
Can we move this abs() to gcd()?
I think that, in general, GCD algorithm assumes that both of inputs are non-neagtive.
There was a problem hiding this comment.
Yes but then, for each binary gcd() called, abs will be done (instead of just once after the n-ary GCD). Also, the abs is still necessary at the end of LCM, since the variable right can be negative in each iteration. Anyway, I will do that.
| x = y % x; | ||
| y = tmp; | ||
| } | ||
| return (y >= 0) ? y : -y; |
There was a problem hiding this comment.
Hmm. Could you add a comment why we use this approach if we want to use this instead of the following? Because the following is the standard implementation and this implementation isn't the standard implementation.
static scm_int_t gcd(scm_int_t x, scm_int_t y)
{
scm_int_t tmp;
if (x < 0)
x = -x;
if (y < 0)
y = -y;
while (x != 0) {
tmp = x;
x = y % x;
y = tmp;
}
return y;
}
|
Could you rebase on master and resolve conflict? |
The algorithm used in GCD is Euclid's. Since R5RS requires it for several arguments, and Sigscheme implements other variable-length procedures as folds, this implementation follows that. Basically, (gcd a1 a2 a3 ...) is implemented as (fold 0 %gcd2 '(a1 a2 a3 ...)) Here, "fold" does not exist - it's written in C, in a similar fashion to other functions (+, /, * etc), and "%gcd2" is a static C function. LCM is implemented as usual using the GCD.
Done! |
|
Thanks. |
2 participants
The algorithm used in GCD is Euclid's. Since R5RS requires it for several arguments, and Sigscheme implements other variable-length procedures as folds, this implementation follows that. Basically,
is implemented as
Here, "fold" does not exist - it's written in C, in a similar fashion to other functions (+, /, * etc), and "%gcd2" is a static C function.
LCM is implemented as usual using the GCD.