RFR: 8337995: ZUtils::fill uses std::fill_n [v2]

[Kim Barrett]

On Wed, 11 Dec 2024 10:11:15 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> I don't really see the need to forbid `std::fill_n`, so I would have preferred an update to the style guide. 

The "approved" HotSpot way to do that operation would be to use something from
the Copy class. But that class has many shortcomings, and really needs some
TLC and to be "modernized" to use templates and the like. But not something
I'm interested in doing today for this specific bit of code.

I considered adding `template<typename T> void Copy::fill_n(T*, size_t, T)`
and using that, but decided futzing with Copy really ought to be it's own
thing. 

It's less about using `std::fill_n` than about `#include <algorithm>`. Once
you permit the latter, it becomes much harder to enforce restrictions. And
various changes we might want to make may render such an include problematic.
I found this bit of code because I was looking for includes of C++ Standard
Library headers in the context of working on improvements to the function
poisoning mechanism. Not all Standard Libraries are as careful about
protecting themselves against outside influence as gcc's. clang's <new>
definitely gets tripped up. I don't remember whether <algorithm> trips
similarly, or if this change was just a preemptive strike.  (<algorithm> is
also a pretty large hammer for this little nail.)

There are approaches to dealing with those sorts of things (mostly "wrapper"
headers), but I'm not interested in going there for this case at this time.
(This issue and the wrapper header technique is mentioned in the Style Guide,
as something that might happen in the future.)

Also, if you think something in the Style Guide is onorous, confusing, or
wrong, feel free to propose a change.

> src/hotspot/share/gc/z/zUtils.cpp line 41:
> 
>> 39:   for (uintptr_t* end = addr + count; addr < end; ++addr) {
>> 40:     *addr = value;
>> 41:   }
> 
> I tend to avoid changing values of the input arguments, so I would like to see that changed. Unless there's a problem with the below code I would like to see this changed to this:
> 
>   for (uintptr_t* current = addr; current < addr + count; ++current) {
>     *current = value;
>   }
> 
> 
> Or maybe even:
> 
>   for (size_t i = 0; i < count; ++i) {
>     *(addr + i) = value;
>   }

Okay. I went with something like the 2nd suggestion, though with array syntax
rather than explict pointer arithmetic.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/22667#issuecomment-2536801139
PR Review Comment: https://git.openjdk.org/jdk/pull/22667#discussion_r1880699772