[5.7] remainder of alignment changes #42580
Conversation
The placement new operator is only available when `<new>` has been included. Add the missing include to the header to allow us to get the definition of the placement new allocator. This allows us to remove the workaround that was there for Windows, and should hopefully repair the Android build as well. Thanks to @grynspan for helping identify the underlying issue! (cherry picked from commit 312f258)
The android build seems to find a conflict in the overload set for the placement new operator due to the custom new overload. Provide the indicator that we want the placement new allocator by using the explicitly namespaced spelling. Thanks to @buttaface for reporting the build failure. (cherry picked from commit ce6d97d)
Include the new header for placement new. (cherry picked from commit c5e99e4)
Apply a blanket pass of including `new` for the placement new allocation and namespacing the call to the global placement new allocator. This should repair the Android ARMv7 builds. (cherry picked from commit a8b0ee2)
…on even when the compiler does not support the C++17 over-aligned new feature (and avoid using new anyway since it might be overridden by something else in the process.) (cherry picked from commit 3f24533)
|
@swift-ci please test |
|
ship it! |
| static inline void swift_cxx_deleteObject(T *ptr) { | ||
| if (ptr) { | ||
| ptr->~T(); | ||
| swift_slowDealloc(ptr, sizeof(T), alignof(T) - 1); |
There was a problem hiding this comment.
There are some evil, evil tabs here.
There was a problem hiding this comment.
Yes, I am offended by the whitespace as well. However, I took the change from main. I'll fix it on main first (and we can pick that up here just for our sensibilities).
There was a problem hiding this comment.
I think that was my fault due to bad IDE settings. Mea culpa!
There was a problem hiding this comment.
FWIW I don't think you need to block this PR on that change.
|
I notice that this pull doesn't include one change from #42376, this scoping, any reason why? If needed, it will have to be adapted to this recently merged change in the 5.7 branch. |
|
@buttaface - you also identified why it was missing - the change was only recently merged. Now this needs updating to include it (as again, you pointed out) :-(. :sigh: Thank you again for being vigilant about this. (I'll try to update this later today) |
|
Hmm, from what I can tell, the new path doesn't have the new placement allocator: https://github.com/apple/swift/pull/42554/files#diff-03ec3b0a524a8a45afffa59620ca14f8b89487743fe498b47a5a8513579e0c7aR1159. I think that this should be fine as is? |
|
I wasn't sure if it was needed, since that pull replaced the old branch but you were still adding the |
|
Ah! Sorry, I misunderstood what you meant. I simply addressed the conflict in the simplest way. The cost of the additional include is low enough that I don't think it matters much, but I could clean it up if you feel strongly about it. |
|
No problem for me. I've noticed that a cherry-pick or rebase will sometimes lose changes without notifying me, so I was simply asking, sounds like that wasn't the case here. |
|
That sounds really rather annoying - silly tools. Thanks for checking - seems like this should be all good! |
AnthonyLatsis
added
🍒 release cherry pick
This pulls in the remainder of the alignment related changes from main to 5.7.
The changes here are syntactic for the most part. The single change that is more than syntax only is the change to add in the new internal wrapper for the placement allocator. This ensures that we always use the proper allocator for Swift and will not be intercepted by the user.
The changes for the alignment should be low risk on all platforms and this has been in main for a week.
Discussed with @mikeash separately and he agrees that this change should be low risk and good to have.
CC: @grynspan @mikeash @airspeedswift