[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!


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54814/new/

https://reviews.llvm.org/D54814





More information about the libcxx-commits mailing list