Bug 15076 - deg2rad has multiple definitions
Summary: deg2rad has multiple definitions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-25 08:09 PDT by Rob Buis
Modified: 2007-10-11 18:58 PDT (History)
0 users

See Also:


Attachments
First attempt (8.58 KB, patch)
2007-08-25 08:14 PDT, Rob Buis
mjs: review-
Details | Formatted Diff | Diff
Proposed patch (10.04 KB, patch)
2007-10-09 23:58 PDT, Andrew Wellington
darin: review-
Details | Formatted Diff | Diff
Proposed patch 2 (10.43 KB, patch)
2007-10-10 17:52 PDT, Andrew Wellington
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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