AK: Add an OptionalBase helper, and allow specializing Optional with dedicated empty values #25894

Hendiadyoin1 · 2025-04-16T22:34:42Z

CC: @LucasChollet
CC: @nico to cross check if apple-clang really supports this or if wikipedia is weird
CC: Whoever knows if the CI changes are correct

Based on LadybirdBrowser/ladybird#2032
But still need to cherry-pick and adapt the String changes

Depends on #25891 for the GCC version bump

Hendiadyoin1 · 2025-04-16T22:38:11Z

Ah wait the CI containers dont ship gcc-14?

spholz · 2025-04-18T19:28:07Z

Meta/build-image-extlinux.sh

@@ -4,6 +4,7 @@ set -e

 script_path=$(cd -P -- "$(dirname -- "$0")" && pwd -P)

+# shellcheck source=/dev/null


IIRC these don't fail in CI, but only in the pre-commit check, because we only pass the changed scripts to shellcheck, which doesn't include shell_include.sh.

So most (if not all) # shellcheck source=/dev/null we already have in master might be unnecessary.

Well it failed CI without the one in this exact file you point out....

Weird, I have no idea why it fails here then.
I'm 99% sure that the build-image-raspberry-pi.sh one only failed in the pre-commit hook.

Hendiadyoin1 · 2025-04-18T19:30:30Z

Whoops, this breaks jakt builds

Hendiadyoin1 · 2025-04-18T19:41:11Z

Also Something in the NNRP code still seems off, will continue to test

This also makes the monadic helpers a bit stricter with their value types, hence the drive-by changes.

This will be needed in the next commit, as Optional then starts to call into Traits, and the affected Ts initialize their own Optional so Traits specialization also already needs to be declared.

Hendiadyoin1 · 2025-04-18T22:25:39Z

AK/Optional.h

+        // FIXME: What to do here when we should not move empties around,
+        //        But don't have a swap method to place it in?


Should I just add an static_assert(dont_move_empty | has_swap) in the mean time?

Hendiadyoin1 · 2025-04-18T22:26:22Z

AK/Optional.h

+    requires(dont_move_empty && has_swap)
+    {
+        VERIFY(has_value());
+        T value = the_empty_value();
+        value.swap(m_value);
+        return value;
+    }


Yay back to shenanigans
(although this looks like a normal move constructor)

Hendiadyoin1 · 2025-04-18T22:27:48Z

AK/Optional.h

+        if (this == &other)
+            return *this;
+        swap(m_value, other.m_value);
+        other.clear();


clear is needed to be clearing on move, which the other optional promises
with non trivial types, which we definitely hit here

Hendiadyoin1 · 2025-04-18T22:28:16Z

AK/Optional.h

+    // Note: Some types don't like to be moved when in their empty state,
+    //       so we need to special case them
+    constexpr static bool dont_move_empty = requires {
+        Traits<T>::optional_dont_move_empty;
+        requires(Traits<T>::optional_dont_move_empty);
+    };
+    // Note: Some types don't like to get their empty values assigned
+    //       but provide a swap method, which usually does not go through
+    //       that check, so let's try to leverage that when we cant move empty values
+    constexpr static bool has_swap = requires(T& t1, T& t2) { t1.swap(t2); };
+
+    // FIXME: We should investigate if we could guess trivial swappability
+    //        based on attributes the T has (see trivially relocatable/replaceable)
+    //        This might also be useful for the normal Optional implementation


Please read during review
especially if you have better ideas

Hendiadyoin1 · 2025-04-18T22:29:37Z

AK/Optional.h

+    ALWAYS_INLINE constexpr void emplace(Ts&&... parameters)
+    {
+        clear();
+        m_value = T(forward<Ts>(parameters)...);


not sure if construct_at is nicer here as m_value is usually in a replaceable state here

Hendiadyoin1 · 2025-04-18T22:29:54Z

AK/Optional.h

+    //        This might also be useful for the normal Optional implementation
+
+public:
+    ALWAYS_INLINE constexpr Optional() {};


Note to self: = default

The new Optional implementation is chosen when a `Traits<T>::special_optional_empty_value` exists, this can be an immediate value or a function optionally taking a Badge<Optional<T>>. You can also define a custom specialization of `Traits<T>::optional_has_value`, as otherwise the default goes through the `==`/`!=` operators, which may not be defined.

Akin to LadybirdBrowser/ladybird@2457118

Hendiadyoin1 force-pushed the OptionalBase branch 2 times, most recently from 48ca83b to fa1f40e Compare April 18, 2025 15:30

Hendiadyoin1 added 2 commits April 18, 2025 20:27

Meta: Fix some shellcheck issues

8e0f96d

CI: Bump Ubuntu runner to 24.04

44e5fe0

Hendiadyoin1 force-pushed the OptionalBase branch from 3f18ba3 to 5654981 Compare April 18, 2025 18:32

Hendiadyoin1 marked this pull request as ready for review April 18, 2025 18:32

Hendiadyoin1 requested a review from alimpfard as a code owner April 18, 2025 18:32

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 18, 2025

Hendiadyoin1 force-pushed the OptionalBase branch from 5654981 to 13f16f2 Compare April 18, 2025 19:22

spholz reviewed Apr 18, 2025

View reviewed changes

Hendiadyoin1 added 4 commits April 18, 2025 21:43

Meta+CI: Bump minimum compiler versions to support deducing-this

514608c

AK: Make Badge constexpr movable

c710e48

AK: Add an OptionalBase helper to ease specializing Optional<T>

3433411

This also makes the monadic helpers a bit stricter with their value types, hence the drive-by changes.

AK: Move some Traits<T> specializations up

a78bea6

This will be needed in the next commit, as Optional then starts to call into Traits, and the affected Ts initialize their own Optional so Traits specialization also already needs to be declared.

Hendiadyoin1 force-pushed the OptionalBase branch 2 times, most recently from b40676b to fd5a34f Compare April 18, 2025 22:24

Hendiadyoin1 commented Apr 18, 2025

View reviewed changes

Hendiadyoin1 added 3 commits April 19, 2025 18:03

AK: Use the Optional special case for nonnull smart pointers

1ef47fe

AK: Use a null value for Optional<(Fly)String>s empty value

4d8dcfe

Akin to LadybirdBrowser/ladybird@2457118

Hendiadyoin1 force-pushed the OptionalBase branch from fd5a34f to 4d8dcfe Compare April 19, 2025 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AK: Add an OptionalBase helper, and allow specializing Optional with dedicated empty values #25894

AK: Add an OptionalBase helper, and allow specializing Optional with dedicated empty values #25894

Hendiadyoin1 commented Apr 16, 2025

Hendiadyoin1 commented Apr 16, 2025

spholz Apr 18, 2025

Hendiadyoin1 Apr 18, 2025

spholz Apr 18, 2025 •

edited

Loading

Hendiadyoin1 commented Apr 18, 2025

Hendiadyoin1 commented Apr 18, 2025

Hendiadyoin1 Apr 18, 2025

Hendiadyoin1 Apr 18, 2025

Hendiadyoin1 Apr 18, 2025

Hendiadyoin1 Apr 18, 2025

Hendiadyoin1 Apr 18, 2025

Hendiadyoin1 Apr 18, 2025

		@@ -4,6 +4,7 @@ set -e

		script_path=$(cd -P -- "$(dirname -- "$0")" && pwd -P)

		# shellcheck source=/dev/null

		// FIXME: What to do here when we should not move empties around,
		// But don't have a swap method to place it in?

AK: Add an OptionalBase helper, and allow specializing Optional with dedicated empty values #25894

Are you sure you want to change the base?

AK: Add an OptionalBase helper, and allow specializing Optional with dedicated empty values #25894

Conversation

Hendiadyoin1 commented Apr 16, 2025

Hendiadyoin1 commented Apr 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spholz Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Hendiadyoin1 commented Apr 18, 2025

Hendiadyoin1 commented Apr 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spholz Apr 18, 2025 •

edited

Loading