AK: Some Variant cleanup after c++26 by Hendiadyoin1 · Pull Request #26547 · SerenityOS/serenity

Hendiadyoin1 · 2026-01-11T13:19:38Z

While c++26 was not required for most of these, it did make it a bit easier at some points

This consists of 2 main parts:

Getting the Variant constructor in a more canonical form,
This is step 1 of getting constexpr working for it, and makes some more ugly parts of it disappear
Cleaning up some redundant parts around visit

CC: @alimpfard as our local template and variant expert

alimpfard · 2026-01-11T14:27:00Z

 consteval IndexType index_of()
 {
-    return VariantIndexOf<T, IndexType, 0, Ts...> {}();
+    // Note: This ignores ambiguous lookups, and instead returns the first match


ambiguous? how can there be an ambiguous lookup?

as in, for index_of<int>(int, int, int) it will always return 0

the only valid index is the first one in any case, MergeAndDeduplicate guarantees that.

Should I drop the comment, or is this fine as is?

Hendiadyoin1 · 2026-01-11T14:37:12Z

Whoops pack indexing only works in gcc-15+ which ubuntu doesnt ship for 22.04, so it's off limits

nico · 2026-01-18T20:32:32Z

Happy to have @alimpfard review this, but in case he's busy: From a distance, commit 3 looks nice and the tests in commit 2 are nice, but commit 2 is about the same amount of code lhs and rhs, so it feels a bit like a sideways change.

alimpfard

Largely LGTM, and I really like that the hacky constructors are gone (yay)

alimpfard · 2026-01-22T14:50:32Z

 // Blank<T> is a unique replacement for T, if T is a duplicate type.
 template<typename T>
 struct Blank {
+    void operator()() const;


This will probably end up a bottleneck if the compiler doesn't inline it, have you checked for perf?

Ok, been a week or two,
but iirc it should never actually be called, especially as it is not implemented, it only exists

Actually, does Blank actually need it?

Ah yeah, its needed as the replacement happens late-ish, so the lookup thing inherits from it and tries to export the operator

alimpfard · 2026-01-22T14:51:38Z

+// NOTE: This always allows narrowing,
+//       The stl version does not allow narrowing conversions
+//       main points where we need it are instantiations with literal 0s,
+//       which we could feasibly check for with some more template magic and is_constant_p.


I think narrowing is fine if it's unambiguous, and I assume this will just complain about ambiguity if it is indeed ambiguous?
Maybe a test about that would be nice to have.

I added tests which would complain if we didnt narrow,
and one that would behave differently if we narrow wrongly (I think)
Not sure how to test true ambiguity except for compilation failures

maybe !requires { ... }? not sure.
just the narrowing tests should be good, that's what I intended anyway.

alimpfard · 2026-01-22T14:55:24Z


+    // FIXME: Not sure why we need the constraint here to avoid recursion,
+    //        The variant should not be able to contain it self, so a constructibility check should
+    //        be enough?


iirc the "recursion" thing was to make sure the compiler wouldn't attempt to use this for perfect forwarding instead of the copy/move ctor (should be possible to name a conversion operator from Variant<T, ...> to T)
I don't think it's actually possible to invoke this with a Variant<Ts...> anyhow, so should be ok to remove that constraint.

Well I tried to remove the constrained, and it didn't like it,
if you have a diff, I'd be happy to apply it

what didn't like it? it builds fine for me

diff --git a/AK/Variant.h b/AK/Variant.h index 3f25043764..d1b0e9252c 100644 --- a/AK/Variant.h +++ b/AK/Variant.h @@ -246,8 +246,7 @@ public: // The variant should not be able to contain it self, so a constructibility check should // be enough? template<typename T> - requires(!IsSame<RemoveCVReference<T>, Variant> - && (IsConstructible<Ts, T> || ...)) + requires(IsConstructible<Ts, T> || ...) Variant(T&& t) { using BestOverload = BestMatch<T>;

/home/leon/serenity/Meta/Lagom/../../AK/Variant.h:249:14: error: satisfaction of constraint 'IsConstructible<Ts, T>' depends on itself 249 | requires(IsConstructible<Ts, T> || ...) | ^~~~~~~~~~~~~~~~~~~~~~ /home/leon/serenity/Meta/Lagom/../../AK/Variant.h:249:14: note: while substituting template arguments into constraint expression here 249 | requires(IsConstructible<Ts, T> || ...) | ^~~~~~~~~~~~~~~~~~~~~~ /home/leon/serenity/Meta/Lagom/../../AK/StdLibExtraDetails.h:481:60: note: while checking constraint satisfaction for template 'Variant<Web::Geometry::ParsedMatrix, Web::WebIDL::SimpleException, JS::NonnullGCPtr<Web::WebIDL::DOMException>, JS::Completion>' required here 481 | inline constexpr bool IsConstructible = requires { ::new T(declval<Args>()...); }; | ^~~~~~~ /home/leon/serenity/Meta/Lagom/../../AK/StdLibExtraDetails.h:481:60: note: while substituting deduced template arguments into function template 'Variant' [with T = const AK::Variant<Web::Geometry::ParsedMatrix, Web::WebIDL::SimpleException, JS::NonnullGCPtr<Web::WebIDL::DOMException>, JS::Completion> &] /home/leon/serenity/Meta/Lagom/../../AK/StdLibExtraDetails.h:481:52: note: in instantiation of requirement here 481 | inline constexpr bool IsConstructible = requires { ::new T(declval<Args>()...); }; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/leon/serenity/Meta/Lagom/../../AK/Variant.h:249:14: note: in instantiation of variable template specialization 'AK::Detail::IsConstructible<Web::Geometry::ParsedMatrix, const AK::Variant<Web::Geometry::ParsedMatrix, Web::WebIDL::SimpleException, JS::NonnullGCPtr<Web::WebIDL::DOMException>, JS::Completion> &>' requested here 249 | requires(IsConstructible<Ts, T> || ...) | ^ /home/leon/serenity/Meta/Lagom/../../AK/Variant.h:249:14: note: while substituting template arguments into constraint expression here 249 | requires(IsConstructible<Ts, T> || ...) | ^~~~~~~~~~~~~~~~~~~~~~ /home/leon/serenity/Meta/Lagom/../../AK/Variant.h:214:8: note: while checking constraint satisfaction for template 'Variant<Web::Geometry::ParsedMatrix, Web::WebIDL::SimpleException, JS::NonnullGCPtr<Web::WebIDL::DOMException>, JS::Completion>' required here 214 | struct Variant { | ^~~~~~~ /home/leon/serenity/Meta/Lagom/../../AK/Variant.h:214:8: note: in instantiation of function template specialization 'AK::Variant<Web::Geometry::ParsedMatrix, Web::WebIDL::SimpleException, JS::NonnullGCPtr<Web::WebIDL::DOMException>, JS::Completion>::Variant<const AK::Variant<Web::Geometry::ParsedMatrix, Web::WebIDL::SimpleException, JS::NonnullGCPtr<Web::WebIDL::DOMException>, JS::Completion> &>' requested here /home/leon/serenity/Userland/Libraries/LibWeb/Geometry/DOMMatrixReadOnly.cpp:46:27: note: in instantiation of template class 'Web::WebIDL::ExceptionOr<Web::Geometry::ParsedMatrix>' requested here 46 | auto result = TRY(parse_dom_matrix_init_string(realm, init_value.get<String>())); | ^ 4 errors generated. [1264/3300] Building CXX object Userland/Libraries/LibWeb/CMakeFiles/LibWeb.dir/HTML/CanvasRenderingContext2D.cpp.o

idk

Huh, compiler difference maybe?
anyway it's fine to keep if it helps I guess.

github-actions · 2026-02-14T02:14:03Z

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!

github-actions · 2026-03-08T02:13:44Z

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!

github-actions · 2026-03-15T02:14:08Z

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

LucasChollet · 2026-03-16T08:33:20Z

@@ -8,6 +8,7 @@



Could you give more details on what's the new approach in the commit description.

Added a description next to the overload resolution helper, as well as in the commit message
not sure how understandable it is, needed to look hard at it for 5 minutes again myself

LucasChollet · 2026-03-16T08:40:54Z

@@ -14,66 +14,49 @@



It seems that the commit is not atomic.

It would be nice to split them in different commits with proper descriptions.
(If some are independents of the previous changes in the PR, I'll more comfortably merge a small Variant PR)

Split it up, though the commits sound more feng-shui now
(possibly to many commata as well)

github-actions · 2026-04-07T02:13:57Z

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!

github-actions · 2026-04-14T02:14:26Z

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

github-actions · 2026-05-27T02:16:19Z

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!

Otherwise we may end up in a delegation loop

alimpfard · 2026-06-05T12:08:28Z

+    // *: This is the reason for the forwarding reference in the arguments
+    template<typename U, typename = T>
+    requires(IsConstructible<T, U>)
+    auto operator()(T, U&&) const -> __IdentityType<T>;


The return type doesn't participate in type resolution anyway, why IdentityType?

To allow ::Type Accesses (thats why the __ version)
But removing that access using the type directly seems to also be fine
(tbh not sure why the STL and implementors love the foo::type things?)

not sure why anyone would care. the STL's foo::type nonsense is just leftover from when there was no templated dependent type aliases.

Sure, removed that for now,
will need to add something like it back, iff I get index pass-through working though

alimpfard · 2026-06-05T12:11:26Z

 // Blank<T> is a unique replacement for T, if T is a duplicate type.
 template<typename T>
 struct Blank {
+    void operator()() const;


This has the wrong shape anyway, it seems to only ever be called with (T, U&&) no?

Well it should only be nameable and its intentionally not callable,
I can make the rejection more explicit if you want/explain why it has not the correct shape

what refers to this thing? why does it need to be named?

InheritFromPacks tries to export it

as it currently replaces the Overload, not the T in Overload

it does try to export it, but for what?
operator()() existing when you expect to call operator()(T, U&&) is pointless, I just don't understand why this needs to exist?

Is it better now, with the explicit disable?
(Note: using XYZ::function does not care for signature only for names)

alimpfard · 2026-06-05T12:12:22Z

-struct Variant
-    : public Detail::MergeAndDeduplicatePacks<Detail::VariantConstructors<Ts, Variant<Ts...>>...> {
+struct Variant {
+    // FIXME: Can we get this to return the index as well?


yes, but there's no point. a single type identifies exactly the variant since we guarantee uniqueness by construction.

Mainly to avoid the repeated lookup, as the contrutor chain lookup should be able to return an index
The current issue is that, when I add an index to the Base of "MergeAndDedup" it changes the template so the dedup does not work, and I have no clue how I make the deduplicator agnostic of that

why should the lookup return an index though? the type is unique and it identifies the position.

But all users want the position, so they do a position look up afterwards, which is a slight compile-time overhead, as we could get that info from the first lookup

Added a commit where I managed to get the index from the initial lookup,
but I needed to change the dedup order, but I am not entirely sure if I did that in the correct manner,
so I would like a check on that before I squash that in

alimpfard · 2026-06-05T12:13:15Z

    }

+    // FIXME: Not sure why we need the `!IsSame` constraint here to avoid recursion,
+    //        The variant should not be able to contain it self, so a constructibility check should


"can be constructed" still ends up reinstantiating this ctor infinitely to typecheck it, is that what you're looking for?

Well last time, your compiler somehow did not need it,
but still no clue

alimpfard · 2026-06-05T12:30:14Z

+    }
+
+    template<size_t I, typename Self>
+    constexpr auto& get(this Self&& self)


this is too permissive, you can get a valid reference to a part of a temporary object with it.

alimpfard · 2026-06-05T12:31:58Z

+};
+
+template<typename Ref, typename U>
+using LikeT = __LikeT<Ref&&, U&>::Type;


If you're not sold on the name, how about CopyCVReference and add volatile to this?

Not quite a copy, more an apply from, as this cannot remove a const

This is reverse engineered from how GCC does it as that looked like the most reasonable way of doing forward_like,
the cppref suggestion had some deduction issues

alimpfard · 2026-06-05T12:32:25Z

+
+static_assert(IsSame<int&, LikeT<long&, int>>);
+static_assert(IsSame<int const&&, LikeT<long const&&, int&>>);
+static_assert(IsSame<int const&, LikeT<long const&, int&&>>);


eh...probably the wrong place for these "tests"?

alimpfard · 2026-06-05T12:33:00Z

 }

+template<typename Ref, typename T>
+constexpr auto&& forward_like(T&& val)


why auto&& over decltype(auto)? is this the signature from the STL?

yeah thats the signature cpprf suggests,
Not entirely sure what the difference between those two is, tbh

alimpfard · 2026-06-05T12:34:33Z

+
+        Helper::construct<BestOverloadIndex>(m_data, forward<T>(value));
+        m_index = BestOverloadIndex;
+


this change doesn't look like it belongs in this commit.

Previously we inherited from a set of constructors, which casted themselves to the final Variant type to initialize the wanted storage, which is very UB. Now we choose the best overload by checking for which Variant member <T> we can call a function taking a <T>, while passing in a fully qualified argument <U>, as passed into the Variant constructor.

No need to look up the index, we can directly check with `IsOneOf`

Also remove users of the old recursive version, which were mostly redundantly checking the index again. The remaining users have been renamed to be a bit more descriptive.

By just accessing the data in the Variant through the normal means, we avoid a bunch of dubious casts.

For constructing the Storage we use a sentinel value (`VariantIndex`) to pass along the desired depth/type into the constructor chain. This is done as we cant easily start the lifetime of the alternatives after the union was instantiated. Upcoming c++26 features should fix this (see trivial unions (P3074R7) and std::start_lifetime (P3726R2)) For move and copy assignment we destroy the active member, and then replace the whole storage, trivial unions may also make this nicer.

Hendiadyoin1 · 2026-06-09T12:09:49Z

-struct Variant
-    : public Detail::MergeAndDeduplicatePacks<Detail::VariantConstructors<Ts, Variant<Ts...>>...> {
+struct Variant {
+    // FIXME: Can we get this to return the index as well?


Added a commit where I managed to get the index from the initial lookup,
but I needed to change the dedup order, but I am not entirely sure if I did that in the correct manner,
so I would like a check on that before I squash that in

Hendiadyoin1 · 2026-06-09T12:10:44Z

 template<size_t I, typename... Ts, size_t... Js, typename... Qs>
 struct InheritFromUniqueEntries<I, ParameterPack<Ts...>, IndexSequence<Js...>, Qs...>
-    : public BlankIfDuplicate<Ts, Conditional<Js <= I, ParameterPack<>, Qs>...>... {
+    : public BlankIfDuplicate<Ts, Conditional<(Js >= I), ParameterPack<>, Qs>...>... {


Is this the correct way of changing the order and has that any other effects, like perf
(The comment above likely needs adjusting)

github-actions · 2026-07-01T02:15:48Z

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!

github-actions Bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jan 11, 2026

alimpfard reviewed Jan 11, 2026

View reviewed changes

Hendiadyoin1 force-pushed the c++26-cleanup branch from f0af688 to 4ff1fe0 Compare January 11, 2026 14:35

Hendiadyoin1 force-pushed the c++26-cleanup branch 2 times, most recently from d32d9bc to 9ede9e3 Compare January 11, 2026 16:25

alimpfard reviewed Jan 22, 2026

View reviewed changes

github-actions Bot added the stale label Feb 14, 2026

Hendiadyoin1 removed the stale label Feb 14, 2026

github-actions Bot added the stale label Mar 8, 2026

github-actions Bot closed this Mar 15, 2026

Hendiadyoin1 reopened this Mar 15, 2026

Hendiadyoin1 force-pushed the c++26-cleanup branch from 9ede9e3 to 0a78b88 Compare March 15, 2026 15:04

github-actions Bot removed the stale label Mar 16, 2026

LucasChollet reviewed Mar 16, 2026

View reviewed changes

Hendiadyoin1 force-pushed the c++26-cleanup branch from 0a78b88 to d11a870 Compare March 17, 2026 00:24

github-actions Bot added the stale label Apr 7, 2026

github-actions Bot closed this Apr 14, 2026

Hendiadyoin1 reopened this Apr 24, 2026

github-actions Bot removed the stale label Apr 25, 2026

Hendiadyoin1 force-pushed the c++26-cleanup branch 4 times, most recently from e124f21 to 8a5828f Compare May 4, 2026 12:37

github-actions Bot added the stale label May 27, 2026

Hendiadyoin1 removed the stale label Jun 1, 2026

LibGUI: Explicitly specify constructor forwarding in custom Variant type

20aa029

Otherwise we may end up in a delegation loop

Hendiadyoin1 force-pushed the c++26-cleanup branch 7 times, most recently from bcda3a0 to 641b230 Compare June 2, 2026 12:03

alimpfard reviewed Jun 5, 2026

View reviewed changes

Hendiadyoin1 added 4 commits June 6, 2026 18:27

AK: Simplify Variant::can_contain

24e159b

No need to look up the index, we can directly check with `IsOneOf`

AK: Simplify Variant type index checking

75511a7

Also remove users of the old recursive version, which were mostly redundantly checking the index again. The remaining users have been renamed to be a bit more descriptive.

AK: Simplify Variants VisitImpl, by passing less

2f6abf2

By just accessing the data in the Variant through the normal means, we avoid a bunch of dubious casts.

Hendiadyoin1 force-pushed the c++26-cleanup branch from 641b230 to 6273909 Compare June 6, 2026 17:28

Hendiadyoin1 added 7 commits June 6, 2026 19:29

AK/StdShim: Add c++23's forward_like and easy type support for it

5217e18

AK: Handle forwarding in Variant a bit better

345575a

AK: Also perfectly forward values in Variant::visit

99570d7

AK: Forbid getting pointers into RValues of Variants

1adcf21

AK: Remove old clang-14 workaround

19ba0f9

AK+FIXME: Index in Variant lookup and forwarding in downcasts

f57855a

Hendiadyoin1 force-pushed the c++26-cleanup branch from 6273909 to f57855a Compare June 9, 2026 12:06

Hendiadyoin1 commented Jun 9, 2026

View reviewed changes

Hendiadyoin1 marked this pull request as draft June 9, 2026 12:13

github-actions Bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 9, 2026

github-actions Bot added the stale label Jul 1, 2026


		Helper::construct<BestOverloadIndex>(m_data, forward<T>(value));
		m_index = BestOverloadIndex;

Uh oh!

Conversation

Hendiadyoin1 commented Jan 11, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hendiadyoin1 commented Jan 11, 2026

Uh oh!

nico commented Jan 18, 2026

Uh oh!

alimpfard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 14, 2026

Uh oh!

github-actions Bot commented Mar 8, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 7, 2026

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

github-actions Bot commented May 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment