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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
