[PATCH] D54814: Move internal usages of `alignof`/`__alignof` to use `_LIBCPP_ALIGNOF`.

Eric Fiselier via Phabricator reviews at reviews.llvm.org
Wed Nov 28 10:17:23 PST 2018

EricWF accepted this revision.
EricWF marked 6 inline comments as done.
EricWF added a comment.
This revision is now accepted and ready to land.

> 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.

Comment at: include/__config:1285
+#ifndef _LIBCPP_CXX03_LANG
+#define _LIBCPP_ALIGNOF(_Tp) alignof(_Tp)
+#elif defined(_LIBCPP_COMPILER_CLANG)
ldionne wrote:
> Can you indent nested `#define`s? I find it very difficult to read our PP code because it often lacks indentation.
OK, but generally I'm pretty hostile to manually formatting anything. If `clang-format` doesn't do it, then I don't normally care.

Comment at: test/libcxx/libcpp_alignof.pass.cpp:29
+  static_assert(_LIBCPP_ALIGNOF(T) == _Alignof(T), "");
ldionne wrote:
> Why is this test useful? Isn't the previous one (for C++11) sufficient?
It checks that `_Alignof` acts like `alignof` which acts like `_LIBCPP_ALIGNOF`. The test is useful in that it would have caught the Clang bug related to this change.

And additional tests never hurt. This was certainly doesn't. 

Comment at: test/support/test_macros.h:118
-#define TEST_ALIGNOF(...) __alignof(__VA_ARGS__)
+# define TEST_ALIGNOF(...) _Alignof(__VA_ARGS__)
ldionne wrote:
> Why not always just `alignof`? Shouldn't we only use that macro in C++11 and above anyway?
We provide almost all of the C++11 library in C++03, C++03 is really just a thing we tolerate. For example, we provide `alignment_of` in C++03. Our allocators pass around the alignment in case we can use aligned new, etc. 

  rCXX libc++



More information about the libcxx-commits mailing list