AK+LibGfx/JPEG2000: Make SyncGenerator request next value eagerly

It seems that `bool has_next() / Y next()` interface is somewhat more
natural compared to `bool is_done() / Optional<Y> next()`.

AK/SyncGenerator.h

@@ -30,10 +30,8 @@ public:
 #ifdef AK_COROUTINE_DESTRUCTION_BROKEN
         // FIXME: Calling `m_handle.destroy()` when a coroutine has not reached its final
         //        suspension point is scary on Clang < 19.
-        while (!m_handle.done()) {
+        while (!m_handle.done())
             m_handle.resume();
-            m_value.~Y();
-        }
 #endif
 
         m_handle.destroy();
@@ -41,6 +39,7 @@ public:
 
     SyncGenerator(SyncGenerator&& other)
         : m_handle(AK::exchange(other.m_handle, {}))
+        , m_value(move(other.m_value))
     {
         if (m_handle)
             m_handle.promise().m_generator = this;
@@ -55,24 +54,16 @@ public:
         return *this;
     }
 
-    bool is_done() const
+    bool has_next() const
     {
-        return m_handle.done();
+        return !m_handle.done();
     }
 
-    Optional<Y> next()
+    Y next()
     {
-        VERIFY(!m_handle.done());
+        Y value = m_value.release_value();
         m_handle.resume();
-        if (m_handle.done()) {
-            return {};
-        } else {
-            ScopeGuard guard = [&] { m_value.~Y(); };
-            // We must return a temporary object for RVO to be guaranteed. And a simpler version of
-            // this branch (Optional<Y> result = m_value; m_value.~Y(); return result; ) won't do
-            // it. See move_count test in TestSyncGenerator.cpp.
-            return Optional<Y> { move(m_value) };
-        }
+        return value;
     }
 
 private:
@@ -82,21 +73,21 @@ private:
             return { std::coroutine_handle<PromiseType>::from_promise(*this) };
         }
 
-        Detail::SuspendAlways initial_suspend() { return {}; }
+        Detail::SuspendNever initial_suspend() { return {}; }
         Detail::SuspendAlways final_suspend() noexcept { return {}; }
 
         void return_void() { }
 
         Detail::SuspendAlways yield_value(Y&& value)
         {
-            new (&m_generator->m_value) Y(move(value));
+            m_generator->m_value = move(value);
             return {};
         }
 
         Detail::SuspendAlways yield_value(Y const& value)
         requires IsCopyConstructible<Y>
         {
-            new (&m_generator->m_value) Y(value);
+            m_generator->m_value = value;
             return {};
         }
 
@@ -112,10 +103,7 @@ private:
     }
 
     std::coroutine_handle<PromiseType> m_handle;
-
-    union {
-        Y m_value;
-    };
+    Optional<Y> m_value;
 };
 
 }

Tests/AK/TestSyncGenerator.cpp

@@ -13,13 +13,13 @@ namespace {
 SyncGenerator<int> generate(Vector<int>& order)
 {
     ScopeGuard guard = [&] {
-        order.append(7);
+        order.append(6);
     };
-    order.append(2);
+    order.append(1);
     co_yield 1;
-    order.append(4);
+    order.append(3);
     co_yield 2;
-    order.append(6);
+    order.append(5);
 }
 
 }
@@ -29,26 +29,21 @@ TEST_CASE(simple)
     Vector<int> order;
 
     auto gen = generate(order);
-    EXPECT(!gen.is_done());
+    EXPECT(gen.has_next());
 
-    order.append(1);
+    order.append(2);
 
     auto result1 = gen.next();
-    order.append(3);
-    EXPECT(!gen.is_done());
+    order.append(4);
+    EXPECT(gen.has_next());
     EXPECT_EQ(result1, 1);
 
     auto result2 = gen.next();
-    order.append(5);
-    EXPECT(!gen.is_done());
+    order.append(7);
+    EXPECT(!gen.has_next());
     EXPECT_EQ(result2, 2);
 
-    auto end = gen.next();
-    order.append(8);
-    EXPECT(gen.is_done());
-    EXPECT(!end.has_value());
-
-    EXPECT_EQ(order, (Vector<int> { 1, 2, 3, 4, 5, 6, 7, 8 }));
+    EXPECT_EQ(order, (Vector<int> { 1, 2, 3, 4, 5, 6, 7 }));
 }
 
 TEST_CASE(move)
@@ -56,17 +51,15 @@ TEST_CASE(move)
     Vector<int> order;
 
     auto gen = generate(order);
-    EXPECT(!gen.is_done());
+    EXPECT(gen.has_next());
 
     EXPECT_EQ(gen.next(), 1);
 
     auto moved_gen = move(gen);
 
+    EXPECT(moved_gen.has_next());
     EXPECT_EQ(moved_gen.next(), 2);
-
-    EXPECT(!moved_gen.next().has_value());
-
-    EXPECT(moved_gen.is_done());
+    EXPECT(!moved_gen.has_next());
 }
 
 namespace {
@@ -113,7 +106,8 @@ TEST_CASE(move_count)
 {
     auto gen = generate2();
     auto result = gen.next();
-    EXPECT_EQ(result->move_count(), 2);
-    EXPECT_EQ(gen.next()->move_count(), 2);
-    EXPECT(!gen.next().has_value());
+    // FIXME: There is no reason this cannot be 2 but for a missing `Optional<T>::operator=(T&&)` overload.
+    EXPECT_EQ(result.move_count(), 3);
+    EXPECT_EQ(gen.next().move_count(), 3);
+    EXPECT(!gen.has_next());
 }

Userland/Libraries/LibGfx/ImageFormats/JPEG2000ProgressionIterators.cpp

@@ -16,19 +16,16 @@ LayerResolutionLevelComponentPositionProgressionIterator::LayerResolutionLevelCo
     , m_generator(generator())
 
 {
-    m_next = m_generator.next();
 }
 
 bool LayerResolutionLevelComponentPositionProgressionIterator::has_next() const
 {
-    return m_next.has_value();
+    return m_generator.has_next();
 }
 
 ProgressionData LayerResolutionLevelComponentPositionProgressionIterator::next()
 {
-    auto result = m_next;
-    m_next = m_generator.next();
-    return result.value();
+    return m_generator.next();
 }
 
 SyncGenerator<ProgressionData> LayerResolutionLevelComponentPositionProgressionIterator::generator()
@@ -57,19 +54,16 @@ ResolutionLevelLayerComponentPositionProgressionIterator::ResolutionLevelLayerCo
     , m_precinct_count(move(precinct_count))
     , m_generator(generator())
 {
-    m_next = m_generator.next();
 }
 
 bool ResolutionLevelLayerComponentPositionProgressionIterator::has_next() const
 {
-    return m_next.has_value();
+    return m_generator.has_next();
 }
 
 ProgressionData ResolutionLevelLayerComponentPositionProgressionIterator::next()
 {
-    auto result = m_next;
-    m_next = m_generator.next();
-    return result.value();
+    return m_generator.next();
 }
 
 SyncGenerator<ProgressionData> ResolutionLevelLayerComponentPositionProgressionIterator::generator()
@@ -112,19 +106,16 @@ ResolutionLevelPositionComponentLayerProgressionIterator::ResolutionLevelPositio
     , m_ll_rect(move(ll_rect))
     , m_generator(generator())
 {
-    m_next = m_generator.next();
 }
 
 bool ResolutionLevelPositionComponentLayerProgressionIterator::has_next() const
 {
-    return m_next.has_value();
+    return m_generator.has_next();
 }
 
 ProgressionData ResolutionLevelPositionComponentLayerProgressionIterator::next()
 {
-    auto result = m_next;
-    m_next = m_generator.next();
-    return result.value();
+    return m_generator.next();
 }
 
 SyncGenerator<ProgressionData> ResolutionLevelPositionComponentLayerProgressionIterator::generator()
@@ -197,19 +188,16 @@ PositionComponentResolutionLevelLayerProgressionIterator::PositionComponentResol
     , m_ll_rect(move(ll_rect))
     , m_generator(generator())
 {
-    m_next = m_generator.next();
 }
 
 bool PositionComponentResolutionLevelLayerProgressionIterator::has_next() const
 {
-    return m_next.has_value();
+    return m_generator.has_next();
 }
 
 ProgressionData PositionComponentResolutionLevelLayerProgressionIterator::next()
 {
-    auto result = m_next;
-    m_next = m_generator.next();
-    return result.value();
+    return m_generator.next();
 }
 
 SyncGenerator<ProgressionData> PositionComponentResolutionLevelLayerProgressionIterator::generator()
@@ -282,19 +270,16 @@ ComponentPositionResolutionLevelLayerProgressionIterator::ComponentPositionResol
     , m_ll_rect(move(ll_rect))
     , m_generator(generator())
 {
-    m_next = m_generator.next();
 }
 
 bool ComponentPositionResolutionLevelLayerProgressionIterator::has_next() const
 {
-    return m_next.has_value();
+    return m_generator.has_next();
 }
 
 ProgressionData ComponentPositionResolutionLevelLayerProgressionIterator::next()
 {
-    auto result = m_next;
-    m_next = m_generator.next();
-    return result.value();
+    return m_generator.next();
 }
 
 SyncGenerator<ProgressionData> ComponentPositionResolutionLevelLayerProgressionIterator::generator()

Userland/Libraries/LibGfx/ImageFormats/JPEG2000ProgressionIterators.h

@@ -42,7 +42,6 @@ public:
 private:
     SyncGenerator<ProgressionData> generator();
 
-    Optional<ProgressionData> m_next;
     int m_layer_count { 0 };
     int m_max_number_of_decomposition_levels { 0 };
     int m_component_count { 0 };
@@ -61,7 +60,6 @@ public:
 private:
     SyncGenerator<ProgressionData> generator();
 
-    Optional<ProgressionData> m_next;
     int m_layer_count { 0 };
     int m_max_number_of_decomposition_levels { 0 };
     int m_component_count { 0 };
@@ -86,7 +84,6 @@ public:
 private:
     SyncGenerator<ProgressionData> generator();
 
-    Optional<ProgressionData> m_next;
     int m_layer_count { 0 };
     int m_max_number_of_decomposition_levels { 0 };
     int m_component_count { 0 };
@@ -119,7 +116,6 @@ public:
 private:
     SyncGenerator<ProgressionData> generator();
 
-    Optional<ProgressionData> m_next;
     int m_layer_count { 0 };
     int m_component_count { 0 };
     Function<int(int resolution_level, int component)> m_precinct_count;
@@ -151,7 +147,6 @@ public:
 private:
     SyncGenerator<ProgressionData> generator();
 
-    Optional<ProgressionData> m_next;
     int m_layer_count { 0 };
     int m_component_count { 0 };
     Function<int(int resolution_level, int component)> m_precinct_count;