RFR: 8312502: Mass migrate HotSpot attributes to the correct location [v2]

[Kim Barrett]

On Sun, 23 Jul 2023 05:50:58 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> Someone had to do it, so I did. Moves attributes to the correct place as specified in the HotSpot Style Guide once and for all
>
> 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/65e01d8a...afff56f2

Why?  What is the benefit from this that makes the resulting code churn
worthwhile?

We already discussed this kind of code churn a bit circa
https://github.com/openjdk/jdk/pull/11081#issuecomment-1313274792
and didn't like it then.  I don't see anything to change that.

The style guide only talks about the new C++ `[[attribute...]]` syntax, which
has a couple of valid locations. These are all gcc `__attribute__` and MSVC
`__declspec`, and are often located in places where the new syntax isn't
permitted. Moving the non-standard "attributes" around has the potential to
change semantics.  I don't know that it does, but this PR should contain
discussion and references to documentation showing it doesn't.

*If* it is to be done, there are some former one-liners that have been made
multi-line by moving an attribute macro, where there were multiple in a
cluster with no blank lines between them.  (Unwritten) HotSpot style only
elides whitespace between declarations when they are all one-liners.

I think I prefer preceding attributes to be on their own line, rather than on
the same line as the declaration, so I can mostly skip over them as I'm
reading the code. But that might be just me; I don't think that's been
discussed by the Group. It probably should have been brought up when
permitting attributes was added to the style guide, but it doesn't look like
that happened.

src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 38:

> 36: #ifdef __GNUC__
> 37: 
> 38: // ISO C++ asm is always implicitly volatile

I can find no evidence for this claim, and it seems to me likely incorrect.  This is also way outside the
described scope of this PR.

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.

src/hotspot/share/compiler/compileLog.hpp line 75:

> 73: 
> 74:   ATTRIBUTE_PRINTF(2, 3)
> 75:   void      set_context(const char* format, ...);

Whitespace between return type and function name is pretty pointless here.  Also above.

src/hotspot/share/utilities/globalDefinitions_gcc.hpp line 161:

> 159: #define NOINLINE     [[gnu::noinline]]
> 160: #define ALWAYSINLINE [[gnu::always_inline]] inline
> 161: #define ATTRIBUTE_FLATTEN [[gnu::flatten]]

This is way beyond the described scope of this PR.

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.

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14969#pullrequestreview-1542234010
PR Review Comment: https://git.openjdk.org/jdk/pull/14969#discussion_r1271430790
PR Review Comment: https://git.openjdk.org/jdk/pull/14969#discussion_r1271431103
PR Review Comment: https://git.openjdk.org/jdk/pull/14969#discussion_r1271431994
PR Review Comment: https://git.openjdk.org/jdk/pull/14969#discussion_r1271432593
PR Review Comment: https://git.openjdk.org/jdk/pull/14969#discussion_r1271433180