[webkit-reviews] review granted: [Bug 189078] [JSC] Build broken after r234975 on s390x, ppc64le, armv7hl : [Attachment 348487] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Tomas Popela <tpopela at redhat.com>'s
request for review:
Bug 189078: [JSC] Build broken after r234975 on s390x, ppc64le, armv7hl
https://bugs.webkit.org/show_bug.cgi?id=189078

Attachment 348487: Patch

https://bugs.webkit.org/attachment.cgi?id=348487&action=review




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 348487
  --> https://bugs.webkit.org/attachment.cgi?id=348487
Patch

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

r=me if EWS bots are happy.

> Source/JavaScriptCore/ChangeLog:8
> +	   Use ternary operator instead of std::max().

I think you should document the gcc bug (that Yusuke pointed out) here e.g.

    std::max is not annotated as constexpr in old GCC libstdc++, which is a bug
since they should be constexpr in C++14.
   
https://github.com/gcc-mirror/gcc/commit/d3ab86117a4b67ca76491e13e4f705cfb9efb7
9e

    This patch works around this bug by using a ternary operator instead of
std::max().

This way:
1. other people reading this code will understand why the ternary operator is
needed here (to work around a bug, not because it's the preferred way to
express this).
2. people will know in the future the motivation for this change, and can
remove it when the workaround is no longer needed.

Technically, one can reverse engineer the reasoning by digging further back to
the comments in this bugzilla.	But the ChangeLog is meant to document the
motivation behind changes when it's not obvious.  So, let's do that.