[WTF] Make MediaTime constructor constexpr
authoraboya@igalia.com <aboya@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Jan 2020 15:22:20 +0000 (15:22 +0000)
committeraboya@igalia.com <aboya@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Jan 2020 15:22:20 +0000 (15:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=206036

Reviewed by Adrian Perez de Castro.

Source/WTF:

https://bugs.webkit.org/show_bug.cgi?id=205723 allowed to declare
MediaTime variables as static inside functions without needing a
global destructor.

It did not eliminate the call to the MediaTime constructor on runtime
though. This wasn't a problem for static variables inside functions,
as the compiler adds a guard variable to call the constructor the
first time the function is called.

On the other hand, for variables defined outside of the scope of the
function, for them to be initialized the MediaTime constructor would
have to be called at runtime from a global constructor, something
we're trying to avoid and which generates an error in clang.

But again, MediaTime is a simple class with only integral values, we
shouldn't need a runtime function call to initialize it!

This patch makes the MediaTime constructor constexpr so that we don't
need runtime initialization for static MediaTime variables. This
allows us to declare them outside functions and enables the compiler
to generate code without guard variables when static MediaTime
variables are declared inside functions.

A test has been added accessing a global const static MediaTime. The
build should not produce any error stating the need for a global
constructor.

* wtf/MediaTime.cpp:
* wtf/MediaTime.h:
(WTF::MediaTime::MediaTime):

Tools:

Added test for global static MediaTime constants.

* TestWebKitAPI/Tests/WTF/MediaTime.cpp:
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@254509 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/MediaTime.cpp
Source/WTF/wtf/MediaTime.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/MediaTime.cpp

index 0ca2deb..45808e6 100644 (file)
@@ -1,3 +1,41 @@
+2020-01-14  Alicia Boya García  <aboya@igalia.com>
+
+        [WTF] Make MediaTime constructor constexpr
+        https://bugs.webkit.org/show_bug.cgi?id=206036
+
+        Reviewed by Adrian Perez de Castro.
+
+        https://bugs.webkit.org/show_bug.cgi?id=205723 allowed to declare
+        MediaTime variables as static inside functions without needing a
+        global destructor.
+
+        It did not eliminate the call to the MediaTime constructor on runtime
+        though. This wasn't a problem for static variables inside functions,
+        as the compiler adds a guard variable to call the constructor the
+        first time the function is called.
+
+        On the other hand, for variables defined outside of the scope of the
+        function, for them to be initialized the MediaTime constructor would
+        have to be called at runtime from a global constructor, something
+        we're trying to avoid and which generates an error in clang.
+
+        But again, MediaTime is a simple class with only integral values, we
+        shouldn't need a runtime function call to initialize it!
+
+        This patch makes the MediaTime constructor constexpr so that we don't
+        need runtime initialization for static MediaTime variables. This
+        allows us to declare them outside functions and enables the compiler
+        to generate code without guard variables when static MediaTime
+        variables are declared inside functions.
+
+        A test has been added accessing a global const static MediaTime. The
+        build should not produce any error stating the need for a global
+        constructor.
+
+        * wtf/MediaTime.cpp:
+        * wtf/MediaTime.h:
+        (WTF::MediaTime::MediaTime):
+
 2020-01-14  David Kilzer  <ddkilzer@apple.com>
 
         Enable -Wconditional-uninitialized in bmalloc, WTF, JavaScriptCore
index 89d2ab7..a3f6252 100644 (file)
@@ -72,24 +72,6 @@ static int64_t signum(int64_t val)
 
 const uint32_t MediaTime::MaximumTimeScale = 1000000000;
 
-MediaTime::MediaTime()
-    : m_timeValue(0)
-    , m_timeScale(DefaultTimeScale)
-    , m_timeFlags(Valid)
-{
-}
-
-MediaTime::MediaTime(int64_t value, uint32_t scale, uint8_t flags)
-    : m_timeValue(value)
-    , m_timeScale(scale)
-    , m_timeFlags(flags)
-{
-    if (scale || isInvalid())
-        return;
-
-    *this = value < 0 ? negativeInfiniteTime() : positiveInfiniteTime();
-}
-
 MediaTime::MediaTime(const MediaTime& rhs)
 {
     *this = rhs;
index b26f5a8..ca9a935 100644 (file)
@@ -53,8 +53,8 @@ public:
         DoubleValue = 1 << 5,
     };
 
-    MediaTime();
-    MediaTime(int64_t value, uint32_t scale, uint8_t flags = Valid);
+    constexpr MediaTime();
+    constexpr MediaTime(int64_t value, uint32_t scale, uint8_t flags = Valid);
     MediaTime(const MediaTime& rhs);
 
     static MediaTime createWithFloat(float floatTime);
@@ -148,6 +148,24 @@ private:
     uint8_t m_timeFlags;
 };
 
+constexpr MediaTime::MediaTime()
+    : m_timeValue(0)
+    , m_timeScale(DefaultTimeScale)
+    , m_timeFlags(Valid)
+{
+}
+
+constexpr MediaTime::MediaTime(int64_t value, uint32_t scale, uint8_t flags)
+    : m_timeValue(value)
+    , m_timeScale(scale)
+    , m_timeFlags(flags)
+{
+    if (scale || !(flags & Valid))
+        return;
+
+    *this = value < 0 ? negativeInfiniteTime() : positiveInfiniteTime();
+}
+
 inline MediaTime operator*(int32_t lhs, const MediaTime& rhs) { return rhs.operator*(lhs); }
 
 WTF_EXPORT_PRIVATE extern MediaTime abs(const MediaTime& rhs);
index b253557..18f1372 100644 (file)
@@ -1,3 +1,15 @@
+2020-01-14  Alicia Boya García  <aboya@igalia.com>
+
+        [WTF] Make MediaTime constructor constexpr
+        https://bugs.webkit.org/show_bug.cgi?id=206036
+
+        Reviewed by Adrian Perez de Castro.
+
+        Added test for global static MediaTime constants.
+
+        * TestWebKitAPI/Tests/WTF/MediaTime.cpp:
+        (TestWebKitAPI::TEST):
+
 2020-01-13  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         Unreviewed sort-Xcode-project-file
index 961bc26..21cdcf0 100644 (file)
@@ -56,6 +56,13 @@ std::ostream& operator<<(std::ostream& out, const MediaTime& val)
 
 namespace TestWebKitAPI {
 
+// MediaTime values should be able to be declared static anywhere, just like you can do so with integers.
+// This should not require global constructors or destructors.
+#pragma clang diagnostic push
+#pragma clang diagnostic error "-Wglobal-constructors"
+static const MediaTime oneSecond(1, 1);
+#pragma clang diagnostic pop
+
 TEST(WTF, MediaTime)
 {
     // Comparison Operators
@@ -97,6 +104,7 @@ TEST(WTF, MediaTime)
     EXPECT_TRUE(!MediaTime::createWithDouble(0.0, 1));
     EXPECT_FALSE(!MediaTime(1, 1));
     EXPECT_FALSE((bool)MediaTime::invalidTime());
+    EXPECT_EQ(MediaTime(10, 10), oneSecond);
 
     // Addition Operators
     EXPECT_EQ(MediaTime::positiveInfiniteTime() + MediaTime::positiveInfiniteTime(), MediaTime::positiveInfiniteTime());