[PATCH] D28785: Split exception.cpp and new.cpp implementation into different files for different runtimes

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/exception:85
 
+#if defined(_LIBCPP_ABI_MICROSOFT)
+#include <vcruntime_exception.h>
----------------
smeenai wrote:
> What's the rationale for relying on Microsoft's exception implementation rather than libc++'s?
`vcruntime_new.h` brings in `vcruntime_exception.h` which defines all of the `exception` symbols as inline. We have no choice but to cede to them.


================
Comment at: include/new:96
+#if defined(_LIBCPP_ABI_MICROSOFT)
+#include <new.h>
+#endif
----------------
smeenai wrote:
> `new.h` will pull in `new` unless you define certain macros. Is that desirable?
That's not how I read the `new.h` header. In MSVC 2015 `new.h` pulls in `vcruntime_new.h` but it also declares `std::new_handler` and `std::set_new_handler`. `<new>` actually avoid declaring certain things if `new.h` has already been included.

`std::get_new_handler` is the only function declared in `<new>` that is not declared in `<new.h>`, however using this function also requires linking to the MSVC C++ STL which we can't do. It's not a great situation to be in, but I don't see how to avoid it.


================
Comment at: include/new:138
 
+typedef void (*new_handler)();
+_LIBCPP_FUNC_VIS new_handler set_new_handler(new_handler) _NOEXCEPT;
----------------
smeenai wrote:
> Again, why defer these to Microsoft's STL? In particular, `set_new_handler` and `get_new_handler` seem to be part of `msvcprt`, which means we would take a runtime dependency on Microsoft's C++ library, which doesn't seem great.
> 
> These functions should map pretty well to `_query_new_handler` and `_set_new_handler` (apart from the different function pointer signature, which can be thunked around), right?
We have to assume these declarations/definitions have already been included via a user including `new.h`, so we can't redefine them. `std::set_new_handler` seem to actually be a part of the CRT startup files, so we can't avoid using it (AFAIK).

> These functions should map pretty well to _query_new_handler and _set_new_handler

Those functions take/return entirely different function types.  So IDK how to turn the function pointer returned from `_query_new_handler` into an entirely different function type and return it from `get_new_handler`, at least not in a meaningful way.


================
Comment at: include/new:177
 
+#if !defined(_LIBCPP_ABI_MICROSOFT)
+
----------------
smeenai wrote:
> Might be helpful to have a comment explaining why we wanna defer these to msvcrt on Windows?
> 
> Also, VS 2015 doesn't seem to have the sized and aligned allocation and deallocation functions. I haven't checked 2017.
You're right that `VS 2015` doesn't have aligned `new/delete`. However until we can correctly implement `get_new_handler` we won't be able to correctly implement the additional aligned `new/delete` overloads.


https://reviews.llvm.org/D28785