Bug 137967

Summary: Use constants from wtf/MathExtras.h
Product: WebKit Reporter: Milan Crha <mcrha>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, cmarcelo, commit-queue, dbarton, esprehn+autocc, fred.wang, glenn, kondapallykalyan, mmaxfield, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 137488    
Attachments:
Description Flags
proposed wk patch
mrobinson: review-
proposed patch ][
none
proposed patch ]I[
none
proposed patch IV
none
proposed patch V none

Description Milan Crha 2014-10-22 10:59:22 PDT
I'm building webkit under mingw/msys and the standard defines like M_PI_2 and others are not provided there directly, but it seems WebKit thinks of it and provides its own definitions of the constants in wtf/MathExtras.h if necessary, thus use these "proxies" instead.
Comment 1 Milan Crha 2014-10-22 11:00:28 PDT
Created attachment 240278 [details]
proposed wk patch
Comment 2 WebKit Commit Bot 2014-10-22 11:03:28 PDT
Attachment 240278 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Martin Robinson 2014-10-22 11:04:31 PDT
Comment on attachment 240278 [details]
proposed wk patch

Looks good, but please fix the style issue. Thanks!
Comment 4 Milan Crha 2014-10-22 11:06:18 PDT
Created attachment 240279 [details]
proposed patch ][

Oops, I missed one chunk in the previous patch.
Comment 5 WebKit Commit Bot 2014-10-22 11:08:44 PDT
Attachment 240279 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Martin Robinson 2014-10-22 11:14:00 PDT
If you use the webkit-patch tool to upload the patch it should point out style errors before the patch hits the bug tracker. You can also use check-webkit-style.
Comment 7 Alexey Proskuryakov 2014-10-22 19:59:33 PDT
It seems like it could be a significant cost to the project if we are going to support a toolchain that violates such basic conventions.

It's probably best to announce your intention at webkit-dev mailing list first to see if there is support for this.

The purpose of the existing definitions is to avoid 64-32 bit conversion errors that some compilers complain about.
Comment 8 Carlos Garcia Campos 2014-10-22 23:12:47 PDT
Comment on attachment 240279 [details]
proposed patch ][

View in context: https://bugs.webkit.org/attachment.cgi?id=240279&action=review

> Source/WTF/ChangeLog:3
> +        Add sqrtOfTwoDouble for M_SQRT2.

Please, use the same bug title in all changelogs. See Tools/Scripts/prepare-ChangeLog, it's recommended to use that tools to generate the changelog entries, it's not only easier for you, but also ensures consistency.
Comment 9 Myles C. Maxfield 2014-10-22 23:56:05 PDT
I agree with Alexey; I don't see much value to redefining constants with different names that, in general, are defined in a standard way. If it were up to me I would get rid of all of these duplicate macros in favor of the standard ones.
Comment 10 Milan Crha 2014-10-23 08:41:49 PDT
Created attachment 240348 [details]
proposed patch ]I[

Updated patch with the style fix.

(In reply to comment #8)
> Please, use the same bug title in all changelogs. See
> Tools/Scripts/prepare-ChangeLog, it's recommended to use that tools to
> generate the changelog entries, it's not only easier for you, but also
> ensures consistency.

I did use the tool, but not with all its features. I regenerated the ChangeLogs with it with used --bug now. I agree it's much simpler than hand-editing all touched ChangeLogs.

(In reply to comment #6)
> If you use the webkit-patch tool to upload the patch it should point out
> style errors before the patch hits the bug tracker. You can also use
> check-webkit-style.

I do not use the webkit-patch, I use only the prepare-ChangeLogs. Looking into its --help it states it also checks the style
>   --[no-]style    Run check-webkit-style script when done (default: style)
but I did not get any style error when the prepare-ChangeLog finished.

(In reply to comment #9)
> I agree with Alexey; I don't see much value to redefining constants with
> different names that, in general, are defined in a standard way. If it were
> up to me I would get rid of all of these duplicate macros in favor of the
> standard ones.

I do not know why mingw/msys doesn't see those constants from math.h or cmath, but it might be similar like bug #137966, some compilers were fine with isnan (probably 'using namespace std;' being "inherited" from another source file) while others require std::isnan(), like the mingw/msys for me. Please do not drop those constants.
Comment 11 Darin Adler 2014-10-24 13:21:44 PDT
Comment on attachment 240348 [details]
proposed patch ]I[

View in context: https://bugs.webkit.org/attachment.cgi?id=240348&action=review

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:76
> +            m_minPreferredLogicalWidth = minPreferredLogicalWidth() * float(sqrtOfTwoDouble);
> +            m_maxPreferredLogicalWidth = maxPreferredLogicalWidth() * float(sqrtOfTwoDouble);

This should be sqrtOfTwoFloat, not float(sqrtOfTwoDouble).
Comment 12 Milan Crha 2014-10-28 09:12:08 PDT
Created attachment 240544 [details]
proposed patch IV
Comment 13 Milan Crha 2014-10-28 09:13:00 PDT
Created attachment 240545 [details]
proposed patch V

Oops, there was a typo in the previous patch.
Comment 14 Darin Adler 2014-10-28 09:26:19 PDT
Comment on attachment 240545 [details]
proposed patch V

It’s OK to do this; it does follow the style already used in MathExtras.h.

Longer term, it would be better to change our approach so that MathExtras.h fills in holes in the implementations of platforms that lack constants like M_SQRT2; I think that it’s better for us to use the standard or de-factor standard names and have MathExtras.h make that work cross-platform rather than adding new new names of our own.

I’m wondering how this is best done in C++. Maybe std::sqrt(2.0) and std::sqrt(2.0f) are intended to be used as constexpr in C++?
Comment 15 WebKit Commit Bot 2014-10-28 11:00:11 PDT
Comment on attachment 240545 [details]
proposed patch V

Clearing flags on attachment: 240545

Committed r175261: <http://trac.webkit.org/changeset/175261>
Comment 16 WebKit Commit Bot 2014-10-28 11:00:16 PDT
All reviewed patches have been landed.  Closing bug.