On Thu, 5 Sep 2024 16:10:05 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) I started commenting on `MEMFLAGS F` => `MemType F` template parameters that needed the parameter name updated, but stopped after a while. There's a bunch more, that should be easy enough to find. I found this very hard to review, esp. after spotting some outright mistakes that forced me to look much more carefully at all the changes. I'd have really preferred seeing this broken up into smaller chunks that weren't quite so soporific. src/hotspot/share/gc/shared/taskqueue.hpp line 119: > 117: // TaskQueueSuper collects functionality common to all GenericTaskQueue > instances. > 118: > 119: template <unsigned int N, MemTag F> MemTag parameter name should probably be changed here and elsewhere in taskqueue code. Suggest `mem_tag`. src/hotspot/share/runtime/lightweightSynchronizer.cpp line 63: > 61: static void* allocate_node(void* context, size_t size, Value const& > value) { > 62: ObjectMonitorTable::inc_items_count(); > 63: return AllocateHeap(size, MemTag::mtObjectMonitor); pre-existing: Why the scope here and below? src/hotspot/share/runtime/os.hpp line 918: > 916: static ssize_t recv(int fd, char* buf, size_t nBytes, uint type); > 917: static ssize_t send(int fd, char* buf, size_t nBytes, uint type); > 918: static ssize_t raw_send(int fd, char* buf, size_t nBytes, uint type); This set of changes is wrong. These aren't MEMFLAGS flags. (I hope there aren't any more like this. This sort of thing would be easy to miss in a change this large. If I were making this change I'd have broken it up into several smaller pieces.) src/hotspot/share/utilities/chunkedList.hpp line 31: > 29: #include "utilities/debug.hpp" > 30: > 31: template <class T, MemTag F> class ChunkedList : public CHeapObj<F> { Parameter name should be updated. Suggest `mem_tag`. src/hotspot/share/utilities/concurrentHashTable.hpp line 43: > 41: class Mutex; > 42: > 43: template <typename CONFIG, MemTag F> Parameter name should be updated throughout ConcurrentHashTable. Suggest mem_tag. src/hotspot/share/utilities/growableArray.hpp line 803: > 801: > 802: // Leaner GrowableArray for CHeap backed data arrays, with compile-time > decided MemTag. > 803: template <typename E, MemTag F> Another parameter needing update, but shouldn't (can't?) be called `mem_tag` because of the function parameter name for allocate(). src/hotspot/share/utilities/linkedlist.hpp line 368: > 366: template <class E, int (*FUNC)(const E&, const E&), > 367: AnyObj::allocation_type T = AnyObj::C_HEAP, > 368: MemTag F = mtNMT, AllocFailType alloc_failmode = > AllocFailStrategy::RETURN_NULL> Another parameter name needing update. src/hotspot/share/utilities/objectBitSet.hpp line 42: > 40: * during the lifetime of the ObjectBitSet. The underlying memory is > allocated from C-Heap. > 41: */ > 42: template<MemTag F> More parameter names needing update. src/hotspot/share/utilities/resizeableResourceHash.hpp line 33: > 31: typename K, typename V, > 32: AnyObj::allocation_type ALLOC_TYPE, > 33: MemTag MEM_TYPE> I think s/MEM_TYPE/mem_type/, but other non-type template parameters here are also all-uppercase, so I guess better to leave it for now and look at it later. ------------- Changes requested by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20872#pullrequestreview-2287424615 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747939080 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747945078 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747946449 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747947967 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747948639 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747950081 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747950278 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747950612 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1747951327
