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

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.


================
Comment at: include/exception:85
 
+#if defined(_LIBCPP_ABI_MICROSOFT)
+#include <vcruntime_exception.h>
----------------
EricWF wrote:
> 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.
Hmm, perhaps we're looking at different versions of the vcruntime headers? My `_VC_CRT_BUILD_VERSION` is 24210 (in `crtversion.h`).

`vcruntime_new.h` doesn't pull in `vcruntime_exception.h` for me:

```
% type headers.cpp
#include <vcruntime_new.h>

% cl /nologo /c /showIncludes headers.cpp
headers.cpp
Note: including file: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vcruntime_new.h
Note: including file:  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vcruntime.h
Note: including file:   C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\sal.h
Note: including file:    C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\ConcurrencySal.h
Note: including file:   C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vadefs.h
```

Additionally, I'm not sure what we need `vcruntime_new.h` for. As far as I can see, it just provides the operator new/delete overload prototypes, which we can provide ourselves.


================
Comment at: include/new:96
+#if defined(_LIBCPP_ABI_MICROSOFT)
+#include <new.h>
+#endif
----------------
EricWF wrote:
> 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.
My `new.h` header (from the 10.0.14393.0 SDK, though 10.0.10586.0 has the same line) contains:

```
    #if !defined _MSC_EXTENSIONS && !defined _CRTBLD && !defined _CORECRT_BUILD
        #include <new>
    #endif
```

`_MSC_EXTENSIONS` should be getting defined by default, which is why we're saved from that include, but I'd be explicit about avoiding that include.

I think it would be ideal to define `_CORECRT_BUILD` so that we avoid any extraneous definitions from `new.h` and just get `_query_new_handler` and `_set_new_handler` (which we can then use to implement `std::get_new_handler` and `std::set_new_handler`). Alternatively, we could just forward declare those two functions ourselves, since I think that's all we care about from that header.


================
Comment at: include/new:138
 
+typedef void (*new_handler)();
+_LIBCPP_FUNC_VIS new_handler set_new_handler(new_handler) _NOEXCEPT;
----------------
EricWF wrote:
> 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.
Hmm, it seems annoying to have to be resilient to users including `<new.h>`. I guess it's technically part of the UCRT and not the MSVCPRT, but considering it can pull in `<new>` or define some standard C++ functions, it doesn't seem unreasonable to ask users to not mix and match `<new.h>` with libc++ headers.

I can think of a bunch of fun non-portable ways to do it, but the only portable way that comes to mind is stowing away the parameter to `std::set_new_handler` and then returning that in `std::get_new_handler`. That's what Microsoft's STL appears to be doing as well.


https://reviews.llvm.org/D28785