RFR: 8313302: Fix formatting errors on Windows [v3]
Julian Waters
jwaters at openjdk.org
Fri Jul 28 10:05:54 UTC 2023
On Fri, 28 Jul 2023 09:46:47 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
> Hi Julian,
>
> I'm sorry, but I object to this change.
>
> It is another heap of aesthetic changes, with very very few changes actually needed (see inline remarks). Contrary to the title, it also touches shared code, and there are (almost) no errors to fix. And the one arguably incorrect place, in code heap, is not a "formatting error on windows".
>
> Please consider that your aesthetic taste is not shared by everyone, but changes like these cause a lot of work for others.
>
> Cheers, Thomas
Hi Thomas,
Sigh, I was afraid of that. I've not been putting out good quality changes lately :(
I will mention that these are not merely aesthetic changes however, but actually is an effort spawned out of JDK-8288293, since all of these are flagged as formatting errors. It may be the case that I'm putting a bit too much faith in gcc's format checking, but I do want to discuss the rest in the review comments since it is a bit easier
> src/hotspot/os/windows/gc/x/xMapper_windows.cpp line 118:
>
>> 116: const uintptr_t addr = map_view_no_placeholder(file_handle, file_offset, size);
>> 117: if (addr == 0) {
>> 118: log_error(gc)("Failed to map view of paging file mapping (%ld)", GetLastError());
>
> Here, and in many other places:
>
> All these `%d` -> `%ld` changes are questionable. Why do you think `l` is more correct?
>
> Strictly spoken both existing and your versions are incorrect since GetLastError returns a DWORD, which is an unsigned 32-bit integer.
>
> Were I to change anything, I would print an unsigned integer. But I would not change anything, since all documented error codes are well below 0x8000_0000. I think we are good here.
DWORD is defined as an unsigned long on all Windows compilers, which more accurately maps to %ld under C++ rules. This was more for correctness, but may not be strictly needed
-------------
PR Comment: https://git.openjdk.org/jdk/pull/15063#issuecomment-1655418084
PR Review Comment: https://git.openjdk.org/jdk/pull/15063#discussion_r1277359872
More information about the hotspot-dev
mailing list