On Sat, 7 Sep 2024 05:24:29 GMT, Kim Barrett <kbarr...@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) > > 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.) Looks like a find/replace issue. Reverted the change. > 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. We can change this later in a followup. > 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(). We can change this later in a followup. > 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. We can change this later in a followup. > 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. I followed the local pattern, but we can change this later in a followup. > 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. Yes, I followed the local pattern. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750606609 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750603957 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750603545 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750603358 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750602937 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750601971
