Page MenuHomePhabricator

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

Authored by dexonsmith on Dec 2 2020, 12:23 PM.

Details

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...>;

Diff Detail

Event Timeline

dexonsmith created this revision.Dec 2 2020, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 12:23 PM
Herald added a subscriber: ributzka. · View Herald Transcript
dexonsmith requested review of this revision.Dec 2 2020, 12:23 PM

@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?

dexonsmith edited the summary of this revision. (Show Details)Dec 2 2020, 1:29 PM
rnk added a subscriber: MaskRay.Dec 2 2020, 1:52 PM

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 added a comment.Dec 2 2020, 1:57 PM

@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 / 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".

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?

@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 accepted this revision.Dec 2 2020, 3:01 PM

lgtm

I checked MSVC 19.14, and this type works.

This revision is now accepted and ready to land.Dec 2 2020, 3:01 PM

@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.

This revision was landed with ongoing or failed builds.Dec 2 2020, 3:56 PM
This revision was automatically updated to reflect the committed changes.
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.