AK: Make Variant constexpr #25025

Hendiadyoin1 · 2024-09-16T12:39:07Z

I am sure the template magic needs cleanup, so roast the code for me

CC: @kleinesfilmroellchen
your small little test works on my end

construct_at has to be defined in the std namespace because outside of it, compilers wouldn't allow it to be constexpr because of its use of placement new.

alimpfard

Just a very cursory look for now, will take a look at the template-y stuff in a bit.

AK/Variant.h

alimpfard · 2024-09-16T12:41:31Z

AK/Variant.h

    static constexpr auto current_index = VariantIndexOf<F, IndexType, InitialIndex, F, Ts...> {}();
-    ALWAYS_INLINE static void delete_(IndexType id, void* data)
+
+    constexpr static void delete_(IndexType state, VariantStorage<F, Ts...>& storage)


with all this de-inlining, please present perf change reports.
Any benchmark should do, variant is sufficiently integrated I think.

Sure, will put ALWAYS_INLINE back, although I think that that is implied by constexpr

inline is implied (on functions), the attribute is not.

AK/Variant.h

This will allow us to make it `constexpr` in the next commits Co-Authored-By: Ali Mohammad Pur <[email protected]>

Co-Authored-By: kleines Filmröllchen <[email protected]>

DanShaders · 2024-09-17T14:41:13Z

AK/StdLibExtras.h

+template<typename T, typename... Args>
+constexpr T* construct_at(T* p, Args&&... args)
+{
+    return new (const_cast<void*>(static_cast<void const*>(p))) T(forward<Args>(args)...);


But if AK_REPLACED_STD_NAMESPACE is AK::replaced_std, this won't be in std namespace.

makes sense,
I think I can get around that by always using the std:: version in AK?
(I hate this function, and cant wait for the c++26 being officially implemented everywhere)

Also CC @nrdmn, as this affects your PR/commit

DanShaders · 2024-09-17T14:44:23Z

AK/Variant.h

+    // Note: Actual destruction is handled by the Variant itself
+    ~VariantStorage() = default;
+    ~VariantStorage()
+    requires(!IsTriviallyDestructible<T> || !(IsTriviallyDestructible<Rest> && ...)) {};


Nit: maybe do (!IsTriviallyDestructible<Rest> || ...) (as it is done above) to not needlessly confuse potential readers. (This definitely confused me for a few seconds)

DanShaders · 2024-09-17T14:46:30Z

AK/Variant.h

+    VariantStorage& operator=(VariantStorage const&) = default;
+
+    // Note: Actual destruction is handled by the Variant itself
+    ~VariantStorage() = default;


I don't immediately see a reason to have a default overload that will be empty for trivially destructible types and an overload for the case of non-trivially destructible types that is explicitly empty but I suppose there's one, so a comment will be nice.

IIRC think =default means something else than {},
But sure I can add a comment explaining how and when a member is supposed to be constructed/destructed

From https://stackoverflow.com/questions/56799129/when-to-make-a-destructor-defaulted-using-default:

Versus Implicit, an explicitly defaulted destructor suppresses move operations, can be declared private, protected, or noexcept(false), and in C++20 can be constrained (but not consteval). Very marginally, it can be declared constexpr to verify that it would be anyway. Declaring it inline doesn’t do anything. (It can also be out of line or virtual, but as stated above that can’t be a reason to use it.)

So there should be no reason for us to declare a defaulted destructor here? (Unless, of course, it doesn't compile without it somehow :))

DanShaders · 2024-09-17T14:47:34Z

AK/Variant.h

+
+template<typename T, typename... Rest>
+union VariantStorage<T, Rest...> {
+    VariantStorage() = default;


See the comment about destructor.

DanShaders · 2024-09-17T14:50:17Z

AK/Variant.h

+    VariantStorage(VariantStorage&&) = default;
+    VariantStorage(VariantStorage const&) = default;
+
+    VariantStorage& operator=(VariantStorage&&) = default;
+    VariantStorage& operator=(VariantStorage const&) = default;


Won't those be invalid for sufficiently non-trivial types?

Those should only get initialized -and there for checked- iff the default comparison operator on Variant proper is used, the non trivial case uses a visit and get_pointer()

Ah, sorry I can't read, thought this was operator==, will check again

Hendiadyoin1 · 2024-09-18T08:47:41Z

Seems like I only ever built lagom with this, so it seems like I missed some cases not present there

Hendiadyoin1 · 2024-09-16T12:53:07Z

AK/Variant.h

    // Note: Make sure not to default-initialize!
    //       VariantConstructors::VariantConstructors(T) will set this to the correct value
    //       So default-constructing to anything will leave the first initialization with that value instead of the correct one.


I think I can remove this comment now...

Hendiadyoin1 · 2024-09-16T12:56:45Z

AK/Variant.h

+    {
+        // This constructor is delayed, so that m_storage and m_index are "live" once
+        // this accesses them.
+        Detail::MergeAndDeduplicatePacks<Detail::VariantConstructors<Ts, Variant<Ts...>>...>(*this, forward<T>(value));


should I replace this with an "explicit Object parameter"¹ member function on the helper?

Footnotes

aka deducing this ↩

Hendiadyoin1 · 2024-09-18T08:41:30Z

AK/StdLibExtras.h

+template<typename T, typename... Args>
+constexpr T* construct_at(T* p, Args&&... args)
+{
+    return new (const_cast<void*>(static_cast<void const*>(p))) T(forward<Args>(args)...);


makes sense,
I think I can get around that by always using the std:: version in AK?
(I hate this function, and cant wait for the c++26 being officially implemented everywhere)

Hendiadyoin1 · 2024-09-18T08:42:03Z

AK/StdLibExtras.h

+template<typename T, typename... Args>
+constexpr T* construct_at(T* p, Args&&... args)
+{
+    return new (const_cast<void*>(static_cast<void const*>(p))) T(forward<Args>(args)...);


Also CC @nrdmn, as this affects your PR/commit

Hendiadyoin1 · 2024-09-18T08:44:29Z

AK/Variant.h

+    VariantStorage& operator=(VariantStorage const&) = default;
+
+    // Note: Actual destruction is handled by the Variant itself
+    ~VariantStorage() = default;


IIRC think =default means something else than {},
But sure I can add a comment explaining how and when a member is supposed to be constructed/destructed

Hendiadyoin1 · 2024-09-18T08:46:12Z

AK/Variant.h

+    VariantStorage(VariantStorage&&) = default;
+    VariantStorage(VariantStorage const&) = default;
+
+    VariantStorage& operator=(VariantStorage&&) = default;
+    VariantStorage& operator=(VariantStorage const&) = default;


Those should only get initialized -and there for checked- iff the default comparison operator on Variant proper is used, the non trivial case uses a visit and get_pointer()

Hendiadyoin1 · 2024-09-18T08:54:40Z

AK/Variant.h

+    VariantStorage(VariantStorage&&) = default;
+    VariantStorage(VariantStorage const&) = default;
+
+    VariantStorage& operator=(VariantStorage&&) = default;
+    VariantStorage& operator=(VariantStorage const&) = default;


Ah, sorry I can't read, thought this was operator==, will check again

stale · 2024-11-05T03:30:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

AK: Add construct_at

e12650a

construct_at has to be defined in the std namespace because outside of it, compilers wouldn't allow it to be constexpr because of its use of placement new.

Hendiadyoin1 requested a review from alimpfard September 16, 2024 12:39

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Sep 16, 2024

Hendiadyoin1 force-pushed the constexpr-Variant branch from 5b09e4e to 1f51879 Compare September 16, 2024 12:44

alimpfard reviewed Sep 16, 2024

View reviewed changes

Hendiadyoin1 and others added 2 commits September 16, 2024 14:48

AK: Use a union as Variants underlying storage

f96acfe

This will allow us to make it `constexpr` in the next commits Co-Authored-By: Ali Mohammad Pur <[email protected]>

AK: Make Variant fully constexpr

7faff15

Co-Authored-By: kleines Filmröllchen <[email protected]>

Hendiadyoin1 force-pushed the constexpr-Variant branch from 1f51879 to 7faff15 Compare September 16, 2024 12:49

DanShaders reviewed Sep 17, 2024

View reviewed changes

Hendiadyoin1 commented Sep 18, 2024

View reviewed changes

WIP: Proper union member activation or so

ccb864e

Hendiadyoin1 marked this pull request as draft September 22, 2024 21:11

github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Sep 22, 2024

Hendiadyoin1 mentioned this pull request Sep 22, 2024

AK/Optional: Make some member functions constexpr #24978

Open

stale bot added the stale label Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AK: Make Variant constexpr #25025

AK: Make Variant constexpr #25025

Hendiadyoin1 commented Sep 16, 2024

alimpfard left a comment

alimpfard Sep 16, 2024

Hendiadyoin1 Sep 16, 2024

alimpfard Sep 16, 2024

DanShaders Sep 17, 2024

Hendiadyoin1 Sep 18, 2024

Hendiadyoin1 Sep 18, 2024

DanShaders Sep 17, 2024

DanShaders Sep 17, 2024

Hendiadyoin1 Sep 18, 2024

DanShaders Sep 27, 2024 •

edited

Loading

DanShaders Sep 17, 2024

DanShaders Sep 17, 2024

Hendiadyoin1 Sep 18, 2024

Hendiadyoin1 Sep 18, 2024

Hendiadyoin1 commented Sep 18, 2024

Hendiadyoin1 Sep 16, 2024

Hendiadyoin1 Sep 16, 2024

Hendiadyoin1 Sep 18, 2024

Hendiadyoin1 Sep 18, 2024

Hendiadyoin1 Sep 18, 2024

Hendiadyoin1 Sep 18, 2024

Hendiadyoin1 Sep 18, 2024

stale bot commented Nov 5, 2024

AK: Make Variant constexpr #25025

Are you sure you want to change the base?

AK: Make Variant constexpr #25025

Conversation

Hendiadyoin1 commented Sep 16, 2024

alimpfard left a 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

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

DanShaders Sep 27, 2024 • edited Loading

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

Hendiadyoin1 commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

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

stale bot commented Nov 5, 2024

DanShaders Sep 27, 2024 •

edited

Loading