This is an archive of the discontinued LLVM Phabricator instance.

ADT: Change AlignedCharArrayUnion to an alias of std::aligned_union_t, NFC
ClosedPublic

Authored by dexonsmith on Dec 2 2020, 2:48 PM.

Details

Summary

Update all the users of AlignedCharArrayUnion to stop peeking inside
(to look at buffer), and then finish gutting it. It's now an alias of
std::aligned_union_t, with a minor difference in template parameters
(std::aligned_union_t takes a minimum size and 0+ types, whereas this
just takes 1+ types... maybe a bit simpler to use correctly?).

A potential follow up would be to remove AlignedCharArrayUnion
entirely, inlining this alias into its users, but it's not clear to me
if that's better.

If https://reviews.llvm.org/D92500 lands without any issue, I think this should be safe as well.

Diff Detail

Event Timeline

dexonsmith created this revision.Dec 2 2020, 2:48 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
dexonsmith requested review of this revision.Dec 2 2020, 2:48 PM

Optionally, I could commit everything but the change to AlignedCharArrayUnion separately, just updating all the users to stop looking inside. If we think there's a chance of a revert that might be better...

rnk accepted this revision.Dec 2 2020, 3:09 PM

Looks good to me.

llvm/include/llvm/Support/ErrorOr.h
256–257 ↗(On Diff #309065)

This union of unions of one type seems silly. I think there can be as single Storage field. Anyway, fixing this is not in scope for this patch.

This revision is now accepted and ready to land.Dec 2 2020, 3:09 PM
dexonsmith updated this revision to Diff 309071.Dec 2 2020, 3:10 PM

Fix some build errors on the clang side of the patch.

rnk added a comment.Dec 2 2020, 3:10 PM

Sorry, racing updates. I agree, landing them together is better, since there is risk that some corner case platform won't have this support.

dblaikie accepted this revision.Dec 2 2020, 3:33 PM
dblaikie added a subscriber: dblaikie.

Yeah, looks good to me - I'd probably go with your suggestion of committing the cleanup pre-emptively, then the alias change. But dealer's choice.

shafik added inline comments.Dec 2 2020, 3:34 PM
clang/include/clang/AST/APValue.h
405 ↗(On Diff #309071)

I notice that in ASTTypeTraits.h we use reinterpret_cast while here we revert to C-style casts.

511 ↗(On Diff #309071)

What is it char * in some cases and void* in others?

dexonsmith added inline comments.Dec 2 2020, 3:39 PM
clang/include/clang/AST/APValue.h
405 ↗(On Diff #309071)

Yes, this old code probably predates C++11. It'd be nice to fix it, but that seems out of scope for this patch.

511 ↗(On Diff #309071)

What is it char * in some cases and void* in others?

Agreed it's inconsistent! I'd have to guess the original authors here had a different ideas about which type was more fundamental.

martong removed a subscriber: martong.Dec 15 2020, 2:50 AM