[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
+#define _LIBCPP_ALIGNOF(_Tp) alignof(_Tp)
> 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), "");
> 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__)
> 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.
CHANGES SINCE LAST ACTION
More information about the libcxx-commits