code review 1686044: complex divide: match C99 implementation

complex divide: match C99 implementation

[rsc, 2010-06-15 20:36:43]

Hello iant, ken2, r (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r2, 2010-06-15 20:40:24]

something else needs to change so that the two cmplxdivide files get compiled
together.

i suggest having cmplxdivide.c print a single file with all the code.

-rob

[rsc, 2010-06-15 20:43:00]

On Tue, Jun 15, 2010 at 13:40,  <r@google.com> wrote:
> something else needs to change so that the two cmplxdivide files get
> compiled together.

they get compiled together by // $G $D/$F.go $D/cmplxdivide1.go
are you saying to avoid that approach?

[r2, 2010-06-15 20:43:59]

On Jun 15, 2010, at 1:42 PM, Russ Cox wrote:

> On Tue, Jun 15, 2010 at 13:40,  <r@google.com> wrote:
>> something else needs to change so that the two cmplxdivide files get
>> compiled together.
>
> they get compiled together by // $G $D/$F.go $D/cmplxdivide1.go
> are you saying to avoid that approach?

ah, i missed that.  it's ok then, but i would have considered writing  
a single file.

-rob

[r2, 2010-06-15 20:47:46]

http://codereview.appspot.com/1686044/diff/5001/6001
File src/pkg/runtime/complex.c (right):

http://codereview.appspot.com/1686044/diff/5001/6001#newcode9
src/pkg/runtime/complex.c:9: void
you should put in a comment explaining the source of the algorithm.
plus: do you need (the effect of) the pragma? in any case if you depend on the
no-compact property you should say so.

http://codereview.appspot.com/1686044/diff/5001/6002
File test/cmplxdivide.c (right):

http://codereview.appspot.com/1686044/diff/5001/6002#newcode57
test/cmplxdivide.c:57: printf("\tTest{cmplx(%s, %s), cmplx(%s, %s), cmplx(%s,
%s)},\n", fmt(creal(n)), fmt(cimag(n)), fmt(creal(d)), fmt(cimag(d)),
fmt(creal(q)), fmt(cimag(q)));
i'm nervous that we trust the C implementation. should we?

http://codereview.appspot.com/1686044/diff/5001/6003
File test/cmplxdivide.go (right):

http://codereview.appspot.com/1686044/diff/5001/6003#newcode1
test/cmplxdivide.go:1: // $G $D/$F.go $D/cmplxdivide1.go && $L $D/$F.$A &&
./$A.out
you say it below but say it here: this file is the driver for cmplxdivide1.go

http://codereview.appspot.com/1686044/diff/5001/6007
File test/zerodivide.go (right):

http://codereview.appspot.com/1686044/diff/5001/6007#newcode160
test/zerodivide.go:160: fmt.Printf("%v/%v: expected %g error; got %g\n", t.f,
t.g, t.out, x)
seems a gratuitous change but OK

[ken3, 2010-06-15 20:59:16]

LGTM

[rsc, 2010-06-15 21:38:08]

> http://codereview.appspot.com/1686044/diff/5001/6001#newcode9
> src/pkg/runtime/complex.c:9: void
> you should put in a comment explaining the source of the algorithm.
> plus: do you need (the effect of) the pragma? in any case if you depend
> on the no-compact property you should say so.

I tried to understand the C99 implementation and gave up.
(It depends on concepts we don't have available like scalbn and logb,
which are like but the same as frexp and ldexp.)
This implementation behaves the same for all the special
cases (verified by the test) and is Ken's original code for
the ordinary case.  It may not be as precise for the arithmetic
and it may not be as efficient, but it is correct.
Having the test means someone can make it more efficient later.

I added two small comments.

> http://codereview.appspot.com/1686044/diff/5001/6002
> File test/cmplxdivide.c (right):
>
> http://codereview.appspot.com/1686044/diff/5001/6002#newcode57
> test/cmplxdivide.c:57: printf("\tTest{cmplx(%s, %s), cmplx(%s, %s),
> cmplx(%s, %s)},\n", fmt(creal(n)), fmt(cimag(n)), fmt(creal(d)),
> fmt(cimag(d)), fmt(creal(q)), fmt(cimag(q)));
> i'm nervous that we trust the C implementation. should we?

Seems okay to me.  It's not automatically generated
so we're not trusting every C implementation, just the
one I ran to generate the test.  If we find out that it's
wrong then we can always do something else, but
all the differences I had to fix were bugs in Go's output, not C's.

> http://codereview.appspot.com/1686044/diff/5001/6003#newcode1
> test/cmplxdivide.go:1: // $G $D/$F.go $D/cmplxdivide1.go && $L $D/$F.$A
> && ./$A.out
> you say it below but say it here: this file is the driver for
> cmplxdivide1.go

Done.

Russ

[r2, 2010-06-15 21:53:20]

LGTM

[iant, 2010-06-15 23:50:30]

LGTM

[rsc, 2010-06-18 22:46:06]

*** Submitted as http://code.google.com/p/go/source/detail?r=6fc6a132f72f ***

complex divide: match C99 implementation

R=iant, ken2, r, r2, ken3
CC=golang-dev
http://codereview.appspot.com/1686044