Add an extra function makeThunkForSameAlternative to force old versions of
gcc to deduce the return value of the overloaded "thunk" being instantiated
before deducing the template arguments for make_array.

I did try to write an offsetof-based version using inheritance instead of the "common initial sequence" rule, but I am not sure how to do the necessary checks statically. The best I can see to do would be to have some dynamic ASSERTs to confirm that casting the alternative type to the base type does not change the underlying representation (i.e. if both pointers are cast to uintptr_t they are equal).

@stella.stamenova do you still think you'd be able to test this before it lands, to see if the current version of the patch works on the MSVC build bots?

Ping

Rebase

• Clean up, rename, and document the VariantTraits interfaces
• Expose IntrusiveVariant::index() as it is already available through the traits
• Fix a bug in IntrusiveVariant comparison functions

In D98477#2798018, @dblaikie wrote:

In D98477#2797129, @scott.linder wrote:

In D98477#2787409, @dblaikie wrote:

In D98477#2783276, @scott.linder wrote:

In D98477#2780702, @dblaikie wrote:

Side thought: If this is going to use a prefix for the discriminator - could that be a base class that all types being used as union members have to derive from? No need for a macro, etc. Could use an offsetof comparison to make sure that not only is it a base class, but the first non-empty base class.

(still haven't been able to easily apply this locally - looks like it hit a conflict with c6f20d70a8c93ab3d50c9777c477fe040460a664 - though I thought arc would sync to the right version to apply the patch... not sure what's going on - might just wait to try that until the other patches have been committed)

Unfortunately having any non-static data members as part of a base class will make the alternative types non-standard-layout, which means we can't use the "common initial sequence" rule to pack them.

But would we need to use the common initial sequence rule? If the base object was at the start of the layout - a pointer to the start of the layout would be a valid pointer to the base object, yeah?

AFAIK that isn't true: we can't assume the base subobject is at the start of the layout of the derived object unless both are standard-layout.

My hope/thought was/is to use offsetof to check that the member is at the start of the object. Though, yes, for maximal portability, offsetof does require standard layout (though it seems both clang and gcc support it at least in some more cases than that: https://godbolt.org/z/Pjr6YEqMq )

In any case - even if it still leaves us with the standard layout restriction - using a base class rather than a macro+member seems like a nicer/more usable solution?

Make Counted::~Counted non-virtual

Rebase and fix uses in PointerUnion.h

In D98477#2787409, @dblaikie wrote:

In D98477#2783276, @scott.linder wrote:

In D98477#2780702, @dblaikie wrote:

Side thought: If this is going to use a prefix for the discriminator - could that be a base class that all types being used as union members have to derive from? No need for a macro, etc. Could use an offsetof comparison to make sure that not only is it a base class, but the first non-empty base class.

(still haven't been able to easily apply this locally - looks like it hit a conflict with c6f20d70a8c93ab3d50c9777c477fe040460a664 - though I thought arc would sync to the right version to apply the patch... not sure what's going on - might just wait to try that until the other patches have been committed)

Unfortunately having any non-static data members as part of a base class will make the alternative types non-standard-layout, which means we can't use the "common initial sequence" rule to pack them.

But would we need to use the common initial sequence rule? If the base object was at the start of the layout - a pointer to the start of the layout would be a valid pointer to the base object, yeah?

In D98477#2780702, @dblaikie wrote:

Side thought: If this is going to use a prefix for the discriminator - could that be a base class that all types being used as union members have to derive from? No need for a macro, etc. Could use an offsetof comparison to make sure that not only is it a base class, but the first non-empty base class.

(still haven't been able to easily apply this locally - looks like it hit a conflict with c6f20d70a8c93ab3d50c9777c477fe040460a664 - though I thought arc would sync to the right version to apply the patch... not sure what's going on - might just wait to try that until the other patches have been committed)

Try to factor out the implementation of visit to be based on specializations
of a type VariantTraits. This should make it possible to share one
implementation with other variant-like types, for example PointerUnion.

Rebase and remove incorrect comments

Rebase

In D98477#2775242, @dblaikie wrote:

Could you reupload this (or maybe you can handle this just by editing the revision here on the web UI?) using arc/phab's ability to flag dependent patches/part of a series, hopefully that'll make it easier to apply the patch to test things? (tried just applying this patch and of course no template named 'TypeAtIndex')

In D100672#2769781, @stella.stamenova wrote:

I'm not sure what to make of it now, and I'm not particularly familiar with Windows or MSVC. I'm still amenable to changing the code, but I also fear the changes I made won't actually fix the issue as I am only testing on godbolt. Is there any shortcut to testing with the Windows buildbots? Or do I have to install Windows somewhere and try to reproduce it?

There is no way to test on the official buildbots, you'd have to install Windows. If you have a change that's ready to test, I could run it locally for you.

Rebase and do some cleanup

I see // TODO: Use GTEST_SKIP() when GTest is updated to version 1.10.0 show up in llvm/unittests/CodeGen/DIEHashTest.cpp, llvm/unittests/ExecutionEngine/Orc/OrcCAPITest.cpp and clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp; would you consider updating those at the same time?

In D100672#2768909, @stella.stamenova wrote:

@dblaikie @scott.linder I updated VS on both the mlir and lldb windows buildbots (from 15.9.30 to 15.9.36), but this update does not have an update to the compiler and indeed the compiler version was already `Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27045```. You can see from the logs of the broken builds as well:

-- The C compiler identification is MSVC 19.16.27045.0
-- The CXX compiler identification is MSVC 19.16.27045.0

I think the issue is rather C++ 14 vs. C++ 17. It is my understanding that C++ 14 is required for LLVM, but not C++ 17, is that the case?

Sorry for the delay in replying, you pose some very good questions and I wanted to revisit the reasoning that led me to my current approach in as thorough a way as possible (which meant it took some time, and I didn't get around to it right away).

Replace constexpr functions with recursively defined templates, working around
an MSVC bug.

This seems like a bug in parameter pack expansion in older versions of MSVC, compare 19.10 https://godbolt.org/z/TMd3K8WoK with 19.14 https://godbolt.org/z/9c9TT8bnb

I also neglected to move the extra element to the end of the array, so I'll make that change when I fix the build failure on Windows

In D100672#2764758, @stella.stamenova wrote:

This broke some windows bots:

https://lab.llvm.org/buildbot/#/builders/13/builds/8180
https://lab.llvm.org/buildbot/#/builders/83/builds/6640

• Update description to reference https://reviews.llvm.org/D99560
• Move code to fall under "Extra additions to <utility>"

Add constexpr convenience variables

Would it be reasonable to just clean up all the types in these functions? I noted the ones I identified.

I'm not sure who else to add, please feel free to update the reviewers

In D100672#2696022, @dblaikie wrote:

Are there (future) STL equivalents of TypesAreDistinct or FirsntIndexOfType?

• Rebase
• Change title, "std::" -> "llvm::"
• Use TYPED_TEST instead of a bespoke macro

Fix a typo in the description

• Rename STLShims -> STLForwardCompat
• Change "STL types" -> "STL features"

In D100671#2695923, @dblaikie wrote:

This usage doesn't seem to quite match the standard - which provides an existing instance of in_place_t for callers to use:

std::optional<std::string> o4(std::in_place, {'a', 'b', 'c'});

(to quote https://en.cppreference.com/w/cpp/utility/optional/optional )

Probably good to match this sort of behavior.

In D100668#2695927, @dblaikie wrote:

I don't think "shim" is the right term here - if anything I'd think things like our range-based adapters to standard algorithms (llvm::sort(R, C) -> std::sort(R.begin(), R.end(), C)) is a "shim" in the sense of the word that I know of - a thin adapter ( https://en.wikipedia.org/wiki/Shim_(computing) - "a shim is a library that transparently intercepts API calls and changes the arguments passed, handles the operation itself or redirects the operation elsewhere.").

These are STL reimplementations, though that's a bit of a mouthful. STLCompat.h, perhaps?

Remove SHIM macro; it wasn't actually doing anything (there was a typo in the
lint identifier), it seems like clang-tidy just ignored the identifier when it
was wrapped in a macro.

I tried to write a version in https://reviews.llvm.org/D100670 that perfectly-forwards each argument to the (overload resolution selected) constructor of the remove_cvref type of the argument. I ended up doing this after I had already started moving some other things out of STLExtras.h, so the patch currently depends on https://reviews.llvm.org/D100670 and https://reviews.llvm.org/D100669 but you can equally well ignore those and makeVisitor can land first.

Moved tests to STLExtrasTest.cpp from STLShimsTest.cpp

I think I ran into a reason to require the visitor we create be able to derive from all of its Callables: it makes it possible to transparently select the correct overload via the normal overload resolution rules, which as of C++14 and the addition of generic lambdas gives the user a way to naturally describe a "default" case.

LGTM, thank you for bearing with me!

Thank you, I have one remaining nit concerning RS and LiveRegs, but otherwise everything LGTM.

In D99561#2672679, @dblaikie wrote:

In D99561#2670269, @scott.linder wrote:

To chime in on the spelling of things relative to std, I had similar questions for IntrusiveVariant in https://reviews.llvm.org/D98477

A natural question that might help with deciding how to handle visit is: how should we spell alternative type access? std uses the free function std::get. It makes sense, as that same free function is overloaded for e.g. std::tuple, std::pair, std::variant, etc. but we don't currently have anything similar in ADT.

I think my leaning would be to go all-in on the std approach by doing the following:

• Add an llvm::adt namespace (or something similar) containing things which are "designed to fulfill the same roles as the identically-named member of the std namespace, but for whatever reason we cannot actually use that member directly". Generally the reason will be: that thing is not an extension point, and it is forbidden to re-open std to overload that thing.

Oh - what would happen if the name went in llvm directly and we called it via adl? ie: made the call unqualified & let ADL+overload resolution sort it out?

I hadn't thought too much about using ADL, as pre-C++20 there is a subtle issue with ADL when the function is a template, see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0846r0.html

In D99560#2667272, @ezhulenev wrote:

std::forward with inheritance does not remove a need to invoke a copy constructors, I managed to implement overloaded using a struct member and super ugly SFINAE that does properly captures lambdas by reference when lambda is a lvalue, however it still does need to copy when lambda is rvalue: https://godbolt.org/z/dqshPW6h1 (top most example, I just count the number of HeavyStuff::HeavyStuff(HeavyStuff const&) copy constructor calls in the generated code). But it does look much worse than inheritance.

To chime in on the spelling of things relative to std, I had similar questions for IntrusiveVariant in https://reviews.llvm.org/D98477

In D98477#2660486, @dblaikie wrote:

Perhaps PointerUnion could be implemented as a derivation from this intrusive variant in a follow-up change, subsuming D99561

In D98477#2660482, @dblaikie wrote:

Looks like this visitor infrastructure has some overlap with D99560 and D99561 - be good to deduplicate some work here, perhaps.

In D88978#2660274, @arsenm wrote:

Is this still needed?

• Address feedback
• Fix bug where last alternative type could not be emplaced
• Misc cleanup

Thank you for taking a look @whisperity! I'm glad there is interest from others in the new type, I was worried at first it may be too niche of a use.

This LGTM, thank you!

Thank you for coming up with a way to test the change! It LGTM.