Using sincos (was Re: [cairo] Questions and optimizations in cairo-arc)

[Owen Taylor]

On Tue, 2005-07-26 at 20:20 -0400, Behdad Esfahbod wrote:
> 
> > > Please feel free to put a patch together for this change alone, (along
> > > with the necessary configure magic to define HAVE_SINCOS
> > > appropriately).
> 
> Infact the check for sincos was already in configure and in one
> place in the code it was conditionally used.  The attached patch
> defines a macro named _cairo_sincos and uses it all over the
> code. (not that many places really)
> 
> I had to introduce AC_GNU_SOURCE in configure.in, as sincos is
> not declared in math.h unless __USE_GNU is defined...  But we do
> want to use GNU extensions, so AC_GNU_SOURCE should be harmless.

It does make it easier for stuff to creep in, but that's hard to
prevent anyways. I'd probably just #define _GNU_SOURCE in cairoint.h
though rather than using a configure check ... it's less complex
and easily amenable to a comment like

#define _GNU_SOURCE               /* Needed for sincos() in math.h */

The AC_CHECK_FUNCS call doesn't use header files, so it doesn't
need the define.

-AC_CHECK_FUNCS(vasnprintf)
-       
+AC_CHECK_FUNCS(vasnprintf, sincos)
+
 AC_CHECK_LIBM

I'd put the sincos check after the libm check ... I'm not sure how
it is working before without libm in LIBS so I don't trust it to
stay working.

+    double sin_, cos_;
+
+    _cairo_sincos (angle / 4, &sin_, &cos_);
+    return 2.0/27.0 * pow (sin_, 6) / pow (cos_, 2);

I don't know Carl's opinion, but I find the sin_, cos_ names sort
of ugly like this. My personal preference would be to use 's'
and 'c' - since the scope of the use is just over two lines, the
short names don't hurt.

-       double dx = radius * cos (reflect ? -theta : theta);
-       double dy = radius * sin (reflect ? -theta : theta);
+       double dx, dy;
+
+       _cairo_sincos (reflect ? -theta : theta, &dx, &dy);
+       dx *= radius;
+       dy *= radius;

I'd use separate variables here - let the compiler do the 
register allocation, it's better at it. :-)

+#if HAVE_SINCOS
+# define _cairo_sincos(angle,s,c) sincos((angle), (s), (c))
+#else
+# define _cairo_sincos(angle,s,c) do { \
+       double _sincos_angle = (angle); \
+       *(s) = sin(_sincos_angle); \
+       *(c) = cos(_sincos_angle); \
+       } while (0)
+#endif

Needs some 'fn((' => fn ()' fixes.

Other than those issues, the patch looks fine to me. If you don't get
to it immediately, you might want to file a bug in bugzilla so we
don't forget.

Regards,
						Owen



-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050822/d899e9a8/attachment-0001.pgp