[libcxx-commits] [PATCH] D158450: [libcxx] Provide set_new_handler/get_new_handler on Windows

[Petr Hosek via Phabricator via libcxx-commits]

phosek added inline comments.


================
Comment at: libcxx/src/new_handler.cpp:33
 
 new_handler get_new_handler() noexcept { return __libcpp_atomic_load(&__new_handler); }
 
----------------
mstorsjo wrote:
> This uses our own implementation of this, while in the linked Discourse post, you mentioned that it probably should use https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/set-new-handler?view=msvc-170 instead. Do you know of any practical pros/cons between the two approaches?
> 
> I guess the MS `_set_new_handler` resides in the CRT, so the setting might be shared across a larger portion of the app (e.g. if we've got multiple statically linked libc++ instances, but we're linking against a shared UCRT).
The issue is that `_set_new_handler` has a slightly different interface from `std::set_new_handler`; specifically the `_set_new_handler` handler is expected to return an integer value (`1` in the case of success), whereas `std::new_handler` doesn't have a return value.

This means that in order to use `_set_new_handler`, we need to use an adapter that invokes the provided `std::new_handler` and then return `1`. There's also no `_get_new_handler` so we need a static global variable to store the provided `std::new_handler`. That's what STL implementation uses as well.

If we had a multiple instances of libc++ inside the process, e.g. multiple shared libraries each statically linking a copy of libc++, each of those libraries would have a different view of what the `new_handler` is which wouldn't match that of `_set_new_handler` and might potentially break logic like https://github.com/llvm/llvm-project/blob/7c4e8c6a273f25b3ef33e9c123b3969632ab59bb/libcxx/src/new.cpp#L34.

With our own implementation of this, every library would still have its own view of what `new_handler` is, but at least that view is internally consistent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158450/new/

https://reviews.llvm.org/D158450