[PATCH] D92500: ADT: Replace guts of AlignedCharArrayUnion with std::aligned_union_t, NFC

Wed Dec 2 15:35:24 PST 2020

dblaikie added a comment.

In D92500#2429552 <https://reviews.llvm.org/D92500#2429552>, @dexonsmith wrote:

> In D92500#2429545 <https://reviews.llvm.org/D92500#2429545>, @dblaikie wrote:
>
>> In D92500#2429186 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92500/new/

https://reviews.llvm.org/D92500