[PATCH] D54814: Move internal usages of `alignof`/`__alignof` to use `_LIBCPP_ALIGNOF`.
Louis Dionne via Phabricator
reviews at reviews.llvm.org
Wed Nov 28 12:00:31 PST 2018
ldionne added a comment.
In D54814#1311642 <https://reviews.llvm.org/D54814#1311642>, @EricWF wrote:
> > Please hold off with committing this until we've agreed on the ABI part. That'll take at least until next week when people are back from Thanksgiving.
> I don't know if there is an ABI part to agree on. Libc++ should have always been using `alignof`. The fact we were using `__alignof` was unobservable until Clang 8. If we release this in libc++ 8, it will continue to appear as if we have always been using `alignof`.
> If users want the old behavior, the use the Clang flag.
> Accepting. Further review can be post-commit review.
FWIW, I agree with the argument that people should use Clang's ABI compat flag if they want to retain ABI compatibility, and that the libc++ change (this one) is largely orthogonal to ABI implications (but the Clang change is a different story). I would have given you a ship-it with that comment even though I'm still considering the ABI implications of the Clang change.
However, I would have appreciated that you held off until I gave you a thumbs up since I did mention explicitly this was a sticky issue. I understand LLVM has a revert-friendly culture, but I feel like discussing reviewers' concerns is a better way of building trust. I just want to make sure we collaborate effectively.
I'm fine with this change, and thanks for the cleanup!
CHANGES SINCE LAST ACTION
More information about the libcxx-commits