Bug 15076

Summary: deg2rad has multiple definitions
Product: WebKit Reporter: Rob Buis <rwlbuis>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P3    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
First attempt
mjs: review-
Proposed patch
darin: review-
Proposed patch 2 eric: review+

Description Rob Buis 2007-08-25 08:09:01 PDT
deg2rad constant is used in multiple places, it would be nice IMHO to have one shared contant.
Comment 1 Rob Buis 2007-08-25 08:14:22 PDT
Created attachment 16118 [details]
First attempt

I think MathExtras.h is the best place for reuse, but am open to other suggestions :)
Cheers,

Rob.
Comment 2 Nikolas Zimmermann 2007-08-26 04:18:17 PDT
Heya Rob,

sounds like a nice attempt to unify these. Some comments:
- I'd rather define two inlined deg2rad / rad2deg functions instead of exposing a #define'd constant.
   and let all callers use these.
- Maybe adding deg2grad, grad2deg is also a good idea, while you're at it. (at least SVG needs it)

Greetings,
Niko
Comment 3 Maciej Stachowiak 2007-08-28 21:22:56 PDT
At the very least, the constant should be defined as a static const double instead of a #define. It does seem like the inline function would read better if used everywhere.


Comment 4 Maciej Stachowiak 2007-08-28 21:23:19 PDT
Comment on attachment 16118 [details]
First attempt

r- to consider me and Niko's comments, but otherwise looks great.
Comment 5 Andrew Wellington 2007-10-09 23:58:29 PDT
Created attachment 16609 [details]
Proposed patch

Proposed patch based on Rob's original patch.
Comment 6 Darin Adler 2007-10-10 07:44:58 PDT
Comment on attachment 16609 [details]
Proposed patch

This patch adds conversions to/from double in code that was previously dealing entirely with floats. That will make the code slower. Instead there should be overloaded versions of these functions for both float and double.
Comment 7 Andrew Wellington 2007-10-10 17:52:50 PDT
Created attachment 16621 [details]
Proposed patch 2

Now with float versions as well
Comment 8 Eric Seidel (no email) 2007-10-11 18:42:00 PDT
Comment on attachment 16621 [details]
Proposed patch 2

This looks great to me.
Comment 9 Andrew Wellington 2007-10-11 18:58:45 PDT
Landed in r26378