vsapsai accepted this revision.
vsapsai added inline comments.
This revision is now accepted and ready to land.
================
Comment at: libcxx/include/new:111-116
#if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606
#define _LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE
#endif
----------------
ldionne wrote:
> vsapsai wrote:
> > Maybe move this to `__config` too? This way we'll have
> > `__cpp_aligned_new`-related macros together.
> The big difference is that
> `_LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE` is only used in `<new>`,
> whereas `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` was used in other files as well.
> Hence, I feel like it makes more sense to lift
> `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` into `<__config>`, but not
> `_LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE `.
Agree with your reasoning.
================
Comment at: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp:11
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
----------------
ldionne wrote:
> vsapsai wrote:
> > Initially `with_system_cxx_lib` made me suspicious because macro
> > `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` doesn't need specific dylib. And
> > `XFAIL: availability=macosx10.12` seems to be working.
> >
> > [Documentation](https://github.com/llvm-mirror/libcxx/blob/master/docs/DesignDocs/AvailabilityMarkup.rst#testing)
> > describes which feature should be used in different cases but in this case
> > I cannot definitely say if test uses unavailable feature. I think it is
> > acceptable to stick with `with_system_cxx_lib` but I decided to brought to
> > your attention the alternative.
> My understanding is that the `availability` feature may not be there if we're
> not using availability macros, since they can be disabled entirely.
`availability` feature [will be
present](https://github.com/llvm-mirror/libcxx/blob/5428c6b9d64dd4cc65fd5665c977c11dc6f8a547/utils/libcxx/test/config.py#L403-L426)
when tests are invoked with `use_system_cxx_lib`. I suspect in this case both
`with_system_cxx_lib` and `availability` will work, so let's keep
`with_system_cxx_lib`.
Repository:
rCXX libc++
https://reviews.llvm.org/D50344
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
