AK: Allow `Optional<T>` to be used in constant expressions #4310

yyny · 2025-04-10T14:10:48Z

Attempt 1: #2455
Related SerenityOS PR: SerenityOS/serenity#25740

ladybird-bot · 2025-04-10T14:11:04Z

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

Hendiadyoin1 · 2025-04-10T14:17:01Z

Also take a look at SerenityOS/serenity#25880
Which has a small fixup for that (may add the same for ErrrorOr)

yyny · 2025-04-11T07:41:39Z

Ladybird's Optional already has optimal move-assignment, I've added your test.

Hendiadyoin1

first look from my side
I'd like
@alimpfard @LucasChollet and @DanShaders
To also have a look

Hendiadyoin1 · 2025-04-11T09:53:43Z

AK/Optional.h

-    ALWAYS_INLINE Optional() = default;
+    ALWAYS_INLINE constexpr Optional()
+    {
+        construct_null_if_necessairy();


As Cxbyte pointed out these are not needed, just add a = {} to the null member

That would unconditionally zero-fill the Optional, which is not necessary at runtime since m_has_value guards against uninitialized access.

AK/Optional.h

Hendiadyoin1 · 2025-04-11T10:00:11Z

AK/Optional.h

+    ALWAYS_INLINE constexpr void construct_null_if_necessairy()
+    {
+        // OPTIMIZATION: Only construct the `m_null` member when we are constant-evaluating.
+        // Otherwise, this generates an unnecessairy zero-fill.
+#if defined(AK_COMPILER_GCC) || defined(HAS_ADDRESS_SANITIZER)
+        // NOTE: GCCs -Wuninitialized warning ends up checking this as well.
+        constexpr bool should_construct = true;
+#else
+        constexpr bool should_construct = is_constant_evaluated();
+#endif
+        if (should_construct)
+            m_null = {};
+    }


See above and CxBytes comments on the serenity PR

I just re-confirmed in Compiler Explorer, initializing m_null does indeed zero fill in cases where it shouldn't have to at -O3 optimizations on both Clang and GCC. I fixed the constexpr bool should_construct = is_constant_evaluated(); though, since that apparently always evaluates to true.

Tests/AK/TestOptional.cpp

AK/Optional.h

Tests/AK/TestOptional.cpp

DanShaders

Looks vaguely fine, I didn't read the diff too carefully tho

ADKaster

For all of these, if we already resolved one way or another on one of the SerenityOS PRs, I'll defer to that decision.

AK/Optional.h

Libraries/LibJS/Runtime/Value.h

Libraries/LibJS/Runtime/Completion.h

AK/Optional.h

ADKaster

I think this is ready, but I'd like a second opinion from @alimpfard

alimpfard

Just a couple notes, otherwise looks nice.

AK/Optional.h

Much like `IsOneOfIgnoringCV`, some functions want to ignore both cv-qualifiers _and_ references. Add a template and concept for it.

This argument is removed (or rather, never added) by `-cc1`, which breaks any builds using GNU libc++, including CI.

Does exactly what it says on the tin.

This matches the behaviour of `std::is_scalar_v`

If a type is non-move constructible but move assignable, its container type may still be move assignable. Similairly, if a type is non-copy constructible but copy assignable, its container type may still be copy assignable.

This will change behaviour for moved-from `Optional<T>`s, since they will now no longer clear their value if `T` is trivial. However, a moved-from value should be considered to be in an unspecified state. Use `Optional<T>::clear` or `Optional<T>::release_value` instead.

alimpfard

LGTM

LibWeb: Remove unuses variable in PaintableBox.cpp

df29a78

yyny force-pushed the constexpr-optional-v2 branch 2 times, most recently from 05fb04b to be6f59f Compare April 11, 2025 07:39

Hendiadyoin1 reviewed Apr 11, 2025

View reviewed changes

yyny force-pushed the constexpr-optional-v2 branch 2 times, most recently from 4bf654a to ea9052d Compare April 11, 2025 14:11

Hendiadyoin1 reviewed Apr 11, 2025

View reviewed changes

AK/Optional.h Outdated Show resolved Hide resolved

Tests/AK/TestOptional.cpp Outdated Show resolved Hide resolved

yyny force-pushed the constexpr-optional-v2 branch 2 times, most recently from b6a4559 to 38376ae Compare April 12, 2025 09:27

DanShaders reviewed Apr 15, 2025

View reviewed changes

ADKaster reviewed Apr 20, 2025

View reviewed changes

AK/Optional.h Outdated Show resolved Hide resolved

AK/Optional.h Outdated Show resolved Hide resolved

Libraries/LibJS/Runtime/Value.h Show resolved Hide resolved

Libraries/LibJS/Runtime/Completion.h Show resolved Hide resolved

AK/Optional.h Show resolved Hide resolved

yyny force-pushed the constexpr-optional-v2 branch 2 times, most recently from 058ee0c to df1190f Compare April 21, 2025 19:29

ADKaster reviewed Apr 22, 2025

View reviewed changes

alimpfard reviewed Apr 22, 2025

View reviewed changes

AK/Optional.h Outdated Show resolved Hide resolved

AK/Optional.h Outdated Show resolved Hide resolved

AK/Optional.h Outdated Show resolved Hide resolved

yyny added 9 commits April 22, 2025 21:46

AK: Add IsOneOfIgnoringCVReference to StdLibExtraDetails.h

6eb23a3

Much like `IsOneOfIgnoringCV`, some functions want to ignore both cv-qualifiers _and_ references. Add a template and concept for it.

AK: Allow Optional<T> to be used in constant expressions

5da0322

AK: Allow Optional<{,Fly}String> to be used in constant expressions

c4cf324

LibJS: Allow Optional<Value> to be used in constant expressions

030ddab

LibJS: Allow Optional<Completion> to be used in constant expressions

62d7f9d

Tests: Fix missing argument removed by -cc1

9b52069

This argument is removed (or rather, never added) by `-cc1`, which breaks any builds using GNU libc++, including CI.

AK: Add IsMemberPointer to StdLibExtraDetails.h

66cc1ab

Does exactly what it says on the tin.

AK: Add IsScalar to StdLibExtraDetails.h

a0bae71

This matches the behaviour of `std::is_scalar_v`

yyny force-pushed the constexpr-optional-v2 branch from df1190f to e8f7dde Compare April 22, 2025 19:47

yyny force-pushed the constexpr-optional-v2 branch from e8f7dde to f6ac15b Compare April 22, 2025 19:49

alimpfard approved these changes Apr 22, 2025

View reviewed changes

ADKaster approved these changes Apr 23, 2025

View reviewed changes

ADKaster merged commit bb20a0d into LadybirdBrowser:master Apr 23, 2025
7 checks passed

Uh oh!

AK: Allow Optional<T> to be used in constant expressions #4310

AK: Allow Optional<T> to be used in constant expressions #4310

Uh oh!

Conversation

yyny commented Apr 10, 2025

Uh oh!

ladybird-bot commented Apr 10, 2025

Uh oh!

Hendiadyoin1 commented Apr 10, 2025

Uh oh!

yyny commented Apr 11, 2025

Uh oh!

Hendiadyoin1 left a comment

Choose a reason for hiding this comment

Uh oh!

Hendiadyoin1 Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

yyny Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hendiadyoin1 Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

yyny Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanShaders left a comment

Choose a reason for hiding this comment

Uh oh!

ADKaster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ADKaster left a comment

Choose a reason for hiding this comment

Uh oh!

alimpfard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alimpfard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AK: Allow `Optional<T>` to be used in constant expressions #4310

AK: Allow `Optional<T>` to be used in constant expressions #4310