On Mon, 17 Apr 2023 17:09:44 GMT, Afshin Zafari <d...@openjdk.org> wrote:
> - The `throw()` (i.e., no throw) specifications are removed from the
> instances of `operator new` where _do not_ return `nullptr`.
>
> - The `-fcheck-new` is removed from the gcc compile flags.
>
> - The `operator new` and `operator delete` are deleted from `StackObj`.
>
> - The `GrowableArrayCHeap::operator delete` is added to be matched with its
> corresponding allocation`AnyObj::operator new`, because gcc complains on that
> after removing the `-fcheck-new` flag.
> - The `Thread::operator new`with and without `null` return are removed.
>
> ### Tests
> local: linux-x64 gtest:GrowableArrayCHeap, macosx-aarch64 hotspot:tier1
> mach5: tiers 1-5
Changes requested by kbarrett (Reviewer).
src/hotspot/share/jfr/utilities/jfrAllocation.hpp line 58:
> 56: NOINLINE void* operator new(size_t size);
> 57: NOINLINE void* operator new (size_t size, const std::nothrow_t&
> nothrow_constant) throw();
> 58: NOINLINE void* operator new [](size_t size);
The changes to JfrCHeapObj are not correct, because these allocators currently
_can_ return null.
Their implementation is just to return the result of calling the non-throwing
allocator. That's probably
not an ideal implementation. Either the declaration needs to be left as-is or
the implementation changed.
src/hotspot/share/memory/allocation.hpp line 287:
> 285: private:
> 286: void* operator new(size_t size) throw() = delete;
> 287: void* operator new [](size_t size) throw() = delete;
The lingering nothrow exception-specs here are just clutter and can be removed.
src/hotspot/share/memory/allocation.hpp line 289:
> 287: void* operator new [](size_t size) throw() = delete;
> 288: void operator delete(void* p) = delete;
> 289: void operator delete [](void* p) = delete;
Making these deleted functions public might provide better error messages if
someone accidentally attempts to reference them.
src/hotspot/share/memory/allocation.hpp line 504:
> 502: // Arena allocations
> 503: void* operator new(size_t size, Arena *arena);
> 504: void* operator new [](size_t size, Arena *arena) = delete;
`operator new[](size_t)` (down below, where github won't let me comment
directly) should also have it's nothrow exception-spec removed.
src/hotspot/share/prims/jvmtiRawMonitor.hpp line 114:
> 112:
> 113: // Non-aborting operator new
> 114: void* operator new(size_t size) {
This change is incorrect, as this can quite obviously return null. And that
seems to be intentional.
Presumably the callers are checking for a possible null allocation result (else
there is a bug). I think
it would be less confusing if this took a `std::nothrow_t` to be explicit about
it's behavior, and updated
the caller(s) accordingly. That would match the usual idiom.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13498#pullrequestreview-1390703118
PR Review Comment: https://git.openjdk.org/jdk/pull/13498#discussion_r1170425313
PR Review Comment: https://git.openjdk.org/jdk/pull/13498#discussion_r1170429604
PR Review Comment: https://git.openjdk.org/jdk/pull/13498#discussion_r1170428457
PR Review Comment: https://git.openjdk.org/jdk/pull/13498#discussion_r1170434730
PR Review Comment: https://git.openjdk.org/jdk/pull/13498#discussion_r1170438594
