vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land.
In https://reviews.llvm.org/D45013#1053998, @EricWF wrote: > In https://reviews.llvm.org/D45013#1052600, @vsapsai wrote: > > > Does it make sense to have a warning for `__libcpp_deallocate` and > > `get_temporary_buffer` too? Not sure about deallocate as you have allocated > > memory already and allocation has a warning. So for deallocation a warning > > can be too noisy. But for `get_temporary_buffer` the warning seems > > reasonable, unless I'm missing something. > > > `get_temporary_buffer` shouldn't return a temporary buffer when the request > is over-aligned and aligned allocation isn't available; so I don't think we > should warn on it. OK. Makes sense. We don't have data indicating it is a widespread issue that developers forget to address, so no need for the warning. Also `get_temporary_buffer` is deprecated in C++17, so there is not much value in investing into it. Overall the change looks good to me, just a few minor NFC tweaks. ================ Comment at: include/memory:2030 #else - if (__is_overaligned_for_new(__alignof(_Tp))) + if (_LIBCPP_IS_OVERALIGNED_FOR_NEW(__alignof(_Tp))) { ---------------- Is the indentation for these 2 ifs correct? They seem to be aligned with `#if` and not with `while`. Don't know if that is intentional. ================ Comment at: include/new:235-239 #ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__ - return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__; +#define _LIBCPP_IS_OVERALIGNED_FOR_NEW(__align) __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__ #else - return __align > alignment_of<max_align_t>::value; +#define _LIBCPP_ISOVERALIGNED_FOR_NEW(__align) __align > _VSTD::alignment_of<_VSTD::max_align_t>::value #endif ---------------- Seems non-critical, maybe put `__align` in parentheses like `(__align) > ...`? But I don't know what is libc++ style regarding macro arguments. Repository: rCXX libc++ https://reviews.llvm.org/D45013 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
