On Sun, 17 Nov 2024 09:22:40 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_new.cpp line 127: >> >>> 125: } >>> 126: #endif >>> 127: >> >> I don't see you commenting on this which is a "huge" deal as it seems like >> it changes memory allocation for a lot of the AWT Windows code. >> This needs careful and analysis and explanation - from you - so reviewers >> can ponder it. >> Also you need to run a lot of tests to verify it. > > This is a "replacement function" for global `operator new`. As the comment > says, it exists to provide the semantics specified by the standard. > Specifically, throwing std::bad_alloc when the allocation fails, because old > versions of the MSVC-provided default implementation didn't do that. That's no > longer true; the default implementation has thrown on allocation failure for a > long time (at least since VS 2015). > > https://learn.microsoft.com/en-us/cpp/cpp/new-and-delete-operators?view=msvc-170 > https://learn.microsoft.com/en-us/cpp/cpp/new-and-delete-operators?view=msvc-140 > > VS documentation discusses replacing that behavior by linking in non-throwing > operator new, but we're not doing that. > https://learn.microsoft.com/en-us/cpp/c-runtime-library/link-options?view=msvc-170 > see nothrownew.obj > > This was also never a correct implementation, since there isn't a > corresponding replacement for `operator delete`. This implementation just > calls malloc and checks the result. Calling the default `operator delete` on > the result of malloc (or anything else that isn't the result of calling the > default `operator new`) is UB; C++14 3.7.4.2/3. Presumably it's been working, > but that's presumably by accident of the MSVC implementation. > > The effect of removing this definition is that the default `operator new` will > be used instead. Doing that instead of calling malloc is potentially somewhat > of a change of behavior. Whether it matters is hard for me to guess. > > Either this replacement definiton should be removed, or a corresponding > `operator delete` must be added. > > Also, can user code be linked into the application using this? If so, it is > generally wrong for a library to provide a replacement definition; the > application might be providing its own. There also isn't a corresponding `operator new[]`. Because of that, the various places that are allocating arrays are already using the default array allocation function, e.g. the C++ allocator, rather than directly using malloc. That also argues for the removal proposed here. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22039#discussion_r1845353808
