Bug 137967 – Use constants from wtf/MathExtras.h

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Milan Crha]

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.

[Milan Crha]

Created attachment 240278 [details]
proposed wk patch

[WebKit Commit Bot]

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.

[Martin Robinson]

Comment on attachment 240278 [details]
proposed wk patch

Looks good, but please fix the style issue. Thanks!

[Milan Crha]

Created attachment 240279 [details]
proposed patch ][

Oops, I missed one chunk in the previous patch.

[WebKit Commit Bot]

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.

[Martin Robinson]

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.

[Alexey Proskuryakov]

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.

[Carlos Garcia Campos]

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.

[Myles C. Maxfield]

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.

[Milan Crha]

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.

[Darin Adler]

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).

[Milan Crha]

Created attachment 240544 [details]
proposed patch IV

[Milan Crha]

Created attachment 240545 [details]
proposed patch V

Oops, there was a typo in the previous patch.

[Darin Adler]

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++?

[WebKit Commit Bot]

Comment on attachment 240545 [details]
proposed patch V

Clearing flags on attachment: 240545

Committed r175261: <http://trac.webkit.org/changeset/175261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.