RFR: 8312502: Mass migrate HotSpot attributes to the correct location [v2]
David Holmes
dholmes at openjdk.org
Sun Jul 23 21:44:59 UTC 2023
On Sun, 23 Jul 2023 11:35:19 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 40 additional commits since the last revision:
>>
>> - Merge branch 'openjdk:master' into patch-6
>> - arguments.hpp
>> - arguments.hpp
>> - globalDefinitions_gcc.hpp
>> - assembler_aarch64.hpp
>> - macroAssembler_aarch64.cpp
>> - vmError.cpp
>> - vmError.cpp
>> - macroAssembler_aarch64.cpp
>> - assembler_aarch64.hpp
>> - ... and 30 more: https://git.openjdk.org/jdk/compare/f058acfd...afff56f2
>
> src/hotspot/share/c1/c1_CFGPrinter.hpp line 66:
>
>> 64: void dec_indent();
>> 65: ATTRIBUTE_PRINTF(2, 3)
>> 66: void print(const char* format, ...);
>
> This is an example where rearranging the attributes is out of character with usual practice.
> And I think it makes it harder to read.
I agree with Kim, I do not like this style of using a new line for the attribute. I also prefer to see these attributes in their original location where I can generally ignore them while reading the code.
> src/hotspot/share/utilities/xmlstream.hpp line 149:
>
>> 147: void text(const char* format, ...);
>> 148: ATTRIBUTE_PRINTF(2, 0)
>> 149: void va_text(const char* format, va_list ap) {
>
> This file is a particularly bad (to me) example of what happens without whitespace between a declaration
> and the attributes for the next declaration. I find this really hard to parse. And the extra whitespace following
> return types makes it even worse for me.
+1
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14969#discussion_r1271561053
PR Review Comment: https://git.openjdk.org/jdk/pull/14969#discussion_r1271561299
More information about the hotspot-gc-dev
mailing list