ADT: Replace guts of AlignedCharArrayUnion with std::aligned_union_t, NFC

[dexonsmith]

Summary

Instead of computing the alignment and size of the char buffer in
AlignedCharArrayUnion, rely on the math in std::aligned_union_t.
Because some users of this rely on the buffer field existing with a
type convertible to char *, we can't change the field type, but we can
still avoid duplicating the logic.

A potential follow up would be to delete AlignedCharArrayUnion after
updating its users to use std::aligned_union_t directly; or if we like
our template parameters better, could update users to stop peeking
inside and then replace the definition with:

template <class T, class... Ts>
using AlignedCharArrayUnion = std::aligned_union_t<1, T, Ts...>;

[dexonsmith]

@rnk / others who know MSVC, any reason this wouldn't be supported yet?

@jfb, any reason you didn't gut this in 80b67baaedd50ded121908f9a4c03f5939b84f24?

@dblaikie / others, any thoughts on whether it's worth keeping our custom template parameters, or if AlignedCharArrayUnion should just be deleted?

[rnk]

I'd suggest looking at the msvc try job output, but it appears to be broken for other reasons. See @MaskRay's is_trivially_copyable changes. So, I'd hold off on this for now until the MSVC build is fixed.

[jfb]

In D92500#2429186, @dexonsmith wrote:

@jfb, any reason you didn't gut this in 80b67baaedd50ded121908f9a4c03f5939b84f24?

Hi Duncan! It seems there were support issues with older MSVC back then:
https://reviews.llvm.org/D65249#change-IWk6CtRl45h6
I don't know if they're still supported (and therefore relevant).

[dblaikie]

In D92500#2429186, @dexonsmith wrote:

@dblaikie / others, any thoughts on whether it's worth keeping our custom template parameters, or if AlignedCharArrayUnion should just be deleted?

Eh, don't have strong feelings - the first length parameter of std::aligned_union_t seems a bit unnecessary, and if most users want to cast this to char* then it's nice to have the char[] member be named/accessible, unlike std::aligned_union_t. But I guess most users just take the address, implicitly convert to void* to pass to placement new? So then it's just that extra 1 parameter we have to pass everywhere? Yeah, I'd probably be OK with that on balance of "standard names will be more recognizable/less in-house quirkiness".

[dexonsmith]

In D92500#2429383, @rnk wrote:

I'd suggest looking at the msvc try job output, but it appears to be broken for other reasons. See @MaskRay's is_trivially_copyable changes. So, I'd hold off on this for now until the MSVC build is fixed.

I don't seem to have permission to restart the builds; do you know if there's a good way to trigger a rebuild now that the main branch is building for MSVC?

[dexonsmith]

In D92500#2429545, @dblaikie wrote:

In D92500#2429186, @dexonsmith wrote:

@dblaikie / others, any thoughts on whether it's worth keeping our custom template parameters, or if AlignedCharArrayUnion should just be deleted?

Eh, don't have strong feelings - the first length parameter of std::aligned_union_t seems a bit unnecessary, and if most users want to cast this to char* then it's nice to have the char[] member be named/accessible, unlike std::aligned_union_t. But I guess most users just take the address, implicitly convert to void* to pass to placement new? So then it's just that extra 1 parameter we have to pass everywhere? Yeah, I'd probably be OK with that on balance of "standard names will be more recognizable/less in-house quirkiness".

I only found one place in LLVM that required a char buffer and it was just doing an unnecessary const_cast<char *>. See the updated bit_cast call near the top of https://reviews.llvm.org/D92512#change-qICtDonOx8i6. (Looks like I might have missed some in clang, will update that revision in a bit...)

[rnk]

lgtm

I checked MSVC 19.14, and this type works.

[dblaikie]

In D92500#2429552, @dexonsmith wrote:

In D92500#2429545, @dblaikie wrote:

In D92500#2429186, @dexonsmith wrote:

@dblaikie / others, any thoughts on whether it's worth keeping our custom template parameters, or if AlignedCharArrayUnion should just be deleted?

Eh, don't have strong feelings - the first length parameter of std::aligned_union_t seems a bit unnecessary, and if most users want to cast this to char* then it's nice to have the char[] member be named/accessible, unlike std::aligned_union_t. But I guess most users just take the address, implicitly convert to void* to pass to placement new? So then it's just that extra 1 parameter we have to pass everywhere? Yeah, I'd probably be OK with that on balance of "standard names will be more recognizable/less in-house quirkiness".

I only found one place in LLVM that required a char buffer and it was just doing an unnecessary const_cast<char *>. See the updated bit_cast call near the top of https://reviews.llvm.org/D92512#change-qICtDonOx8i6. (Looks like I might have missed some in clang, will update that revision in a bit...)

Yeah, sounds good then. Once it's down to an alias versus using the standard name everywhere and having the quirky "1, " at the start of the list - I really don't have strong feelings either way. If you/someone else does and you write the patch, I'd be happy to approve it. If it doesn't happen, I think I'll be comfortable with the name as-is too.

[dexonsmith]

In D92500#2429554, @rnk wrote:

lgtm

I checked MSVC 19.14, and this type works.

Thanks for doing that; pushed / committed this in 65c5c9f92ec514ae41c8ea407d1c885737d699ec. I'll avoid piling anything else on top until tomorrow to give extra time for a clean revert in case there's some unexpected case.

llvm/include/llvm/Support/AlignOf.h

 //===--- AlignOf.h - Portable calculation of type alignment -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines the AlignedCharArrayUnion class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_ALIGNOF_H
 #define LLVM_SUPPORT_ALIGNOF_H
 
-#include "llvm/Support/Compiler.h"
-#include <cstddef>
+#include <type_traits>
 
 namespace llvm {
 
-namespace detail {
-
-template <typename T, typename... Ts> class AlignerImpl {
-  T t;
-  AlignerImpl<Ts...> rest;
-  AlignerImpl() = delete;
-};
-
-template <typename T> class AlignerImpl<T> {
-  T t;
-  AlignerImpl() = delete;
-};
-
-template <typename T, typename... Ts> union SizerImpl {
-  char arr[sizeof(T)];
-  SizerImpl<Ts...> rest;
-};
-
-template <typename T> union SizerImpl<T> { char arr[sizeof(T)]; };
-} // end namespace detail
-
 /// A suitably aligned and sized character array member which can hold elements
 /// of any type.
 ///
 /// These types may be arrays, structs, or any other types. This exposes a
 /// `buffer` member which can be used as suitable storage for a placement new of
 /// any of these types.
 template <typename T, typename... Ts> struct AlignedCharArrayUnion {
-  alignas(::llvm::detail::AlignerImpl<T, Ts...>) char buffer[sizeof(
-      llvm::detail::SizerImpl<T, Ts...>)];
+  using AlignedUnion = std::aligned_union_t<1, T, Ts...>;
+  alignas(alignof(AlignedUnion)) char buffer[sizeof(AlignedUnion)];
 };
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_ALIGNOF_H