Page MenuHomePhabricator

[NFC] use C++11 in AlignOf.h, remove AlignedCharArray
ClosedPublic

Authored by jfb on Jul 24 2019, 4:01 PM.

Details

Reviewers
rnk
Summary

I removed all uses of AlignedCharArray since the minimum MSVC version can handle alignas on char arrays correctly. We can therefore remove AlignedCharArray.

This patch also updates AlignedCharArrayUnion to use C++11.

Event Timeline

jfb created this revision.Jul 24 2019, 4:01 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 24 2019, 4:01 PM
jfb added a comment.Jul 24 2019, 4:03 PM

This is the main event: https://reviews.llvm.org/D65249#change-IWk6CtRl45h6
The rest is side-show.

jfb added a subscriber: nlewycky.Jul 25 2019, 10:05 AM
rnk added a comment.Jul 26 2019, 1:41 PM

I still think this concern applies:
https://reviews.llvm.org/D64417#1576675

I'm not sure how to track down an 19.11 release to test if we can pass std::aligned_storage values through function calls on x86. We might be better off raising the minimum to 19.14, which is a more recent MSVC release in the 2017 track. I don't think it's too much to ask developers to use the most recent version of the 2017 compiler, they won't have to change IDEs.

jfb added a subscriber: BillyONeal.Jul 26 2019, 1:55 PM
In D65249#1603133, @rnk wrote:

I still think this concern applies:
https://reviews.llvm.org/D64417#1576675

I'm not sure how to track down an 19.11 release to test if we can pass std::aligned_storage values through function calls on x86. We might be better off raising the minimum to 19.14, which is a more recent MSVC release in the 2017 track. I don't think it's too much to ask developers to use the most recent version of the 2017 compiler, they won't have to change IDEs.

@BillyONeal do you know if 19.11 has the aligned_storage issue on x86?

If it does then indeed we should raise the minimum version, I'll send an RFC.

@BillyONeal do you know if 19.11 has the aligned_storage issue on x86?

aligned_storage is still capped at what the library can fake with unions. It would be an ABI break to change it to use alignas, so things where the x86 stack temporarily breaks T's usual alignment rules will affect it (and for all releases using VS2015's ABI). We recommend customers use alignas(T) unsigned char space[sizeof(T)] if they can't tolerate such limitations.

jfb added a comment.Jul 26 2019, 4:30 PM

@BillyONeal do you know if 19.11 has the aligned_storage issue on x86?

aligned_storage is still capped at what the library can fake with unions. It would be an ABI break to change it to use alignas, so things where the x86 stack temporarily breaks T's usual alignment rules will affect it (and for all releases using VS2015's ABI). We recommend customers use alignas(T) unsigned char space[sizeof(T)] if they can't tolerate such limitations.

Thanks!

@rnk: how about I add a bit of code that wraps aligned_storage on all platforms except MSVC (where I'd implement it as Billy suggests). That would mean updating all the uses of aligned_storage to this LLVM one, but that's not a big deal.

In D65249#1603335, @jfb wrote:

@rnk: how about I add a bit of code that wraps aligned_storage on all platforms except MSVC (where I'd implement it as Billy suggests). That would mean updating all the uses of aligned_storage to this LLVM one, but that's not a big deal.

Why not just always use the alignas version? Or even better, not involve a library for something you can say in the core language at all?

(In fact I observe many patterns in this review like:

enum { Foo = alignof(void*); }
aligned_storage_t<1234, Foo> x;

and then a bunch of casting to treat it as a char buffer; if it was just born as a char buffer you can remove both the casts and the enum hack:

alignas(void*) char x[1234];

jfb updated this revision to Diff 212252.Jul 29 2019, 4:58 PM

Significantly simplify the patch.

jfb retitled this revision from [NFC] use C++11 in AlignOf.h to [NFC] use C++11 in AlignOf.h, remove AlignedCharArray.Jul 29 2019, 4:59 PM
jfb edited the summary of this revision. (Show Details)
jfb removed a reviewer: chandlerc.
jfb added a subscriber: chandlerc.

(In fact I observe many patterns in this review like:

enum { Foo = alignof(void*); }
aligned_storage_t<1234, Foo> x;

and then a bunch of casting to treat it as a char buffer; if it was just born as a char buffer you can remove both the casts and the enum hack:

alignas(void*) char x[1234];

Good point. I went ahead and did that. The patch is now quite trivial :)

rnk accepted this revision.Jul 29 2019, 5:39 PM

I have concerns that some of the patches that you landed prior to this will cause issues with old versions of MSVC, but in isolation, this is fine, and if anyone complains, we can address it on a case-by-case basis. lgtm

This revision is now accepted and ready to land.Jul 29 2019, 5:39 PM
jfb added a comment.Jul 29 2019, 9:01 PM
In D65249#1605523, @rnk wrote:

I have concerns that some of the patches that you landed prior to this will cause issues with old versions of MSVC, but in isolation, this is fine, and if anyone complains, we can address it on a case-by-case basis. lgtm

IIUC Billy correctly, the issue was just with aligned_storage, but if you roll your own (as I have) we're fine. If I'm wrong, these tests will fail on MSVC bots :)