Bug 15076

Summary:

deg2rad has multiple definitions

Product:

WebKit

Reporter:

Rob Buis <rwlbuis>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

Priority:

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+

Rob Buis

Reported 2007-08-25 08:09:01 PDT

deg2rad constant is used in multiple places, it would be nice IMHO to have one shared contant.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Rob Buis

Comment 1 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.

Nikolas Zimmermann

Comment 2 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

Maciej Stachowiak

Comment 3 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.

Maciej Stachowiak

Comment 4 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.

Andrew Wellington

Comment 5 2007-10-09 23:58:29 PDT

Created attachment 16609 [details] Proposed patch Proposed patch based on Rob's original patch.

Darin Adler

Comment 6 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.

Andrew Wellington

Comment 7 2007-10-10 17:52:50 PDT

Created attachment 16621 [details] Proposed patch 2 Now with float versions as well

Eric Seidel (no email)

Comment 8 2007-10-11 18:42:00 PDT

Comment on attachment 16621 [details] Proposed patch 2 This looks great to me.

Andrew Wellington

Comment 9 2007-10-11 18:58:45 PDT

Landed in r26378

Note You need to log in before you can comment on or make changes to this bug.