Propose to use clang-format to enforce hotspot codestyle

[Jesper Wilhelmsson]

> On 11 Mar 2020, at 13:06, Volker Simonis <volker.simonis at gmail.com> wrote:
> 
> Liu, Xin <xxinliu at amazon.com <mailto:xxinliu at amazon.com>> schrieb am Mi., 11. März 2020, 09:50:
> 
>> Hi,
>> 
>> I just filed a JBS issue JDK-8240834 about hotspot codestyle(
>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide). I found many
>> inconsistent code in hotspot so I propose to use clang-format[1] to enforce
>> it.
>> 
>> I know you’re all very experienced developers, but sometimes wrong
>> indentations or tailing whitespaces creep in.  it's hard to catch them in
>> webrev.  Even though we can, new contributors still need to contact their
>> sponsors for formatting changes.   I think it's a good idea to have lint
>> thing done at first place.
>> 
>> Clang is powerful enough to parse modern C/C++ code and it's very handy to
>> format code. clang-format should be available on all supporting platforms.
>> It's still okay even if it is absent on some platforms because nightly
>> build and submit repo will cover unsupported platforms.
>> 
>> I plan to have  a .clang-format and correcting styles in the 1st patch. We
>> can bring auto clang-format check to hotspot's makefile in the second
>> patch.  Does it sound like a plan?
>> 
> 
> Have you already checked how big this first change will be? I'm afraid it
> will be huge because we have a lot of manually formatted code which doesn't
> confirm to any specific style (e.g. macros). We also have consistent but
> different code styles in various parts of HS. So changing all the sources
> to conform to a single style will necessarily be quite invasive and I'm not
> sure if we want that?
> 

In theory I support using a formatting tool to maintain a single code style throughout the hotspot code, but in practice this is extremely difficult. Volker brings up one point here, the situation today is bad - really bad - from a formatter's perspective. The size of the initial change will be huge. To review that requires people from all affected areas to look in detail on their code to make sure nothing is broken.  Andrew brings up a different point in his mail; parts of the code is manually formatted for a reason. No automatic formatter will be able to make sense of all code if it applies the same rules all over.

I have looked into this problem for quite some time and I believe we have to take a very slow approach to this. Not only to be able to actually review the changes, but also to keep our developers around. First, any changes we make to clean up formatting must be automatically verified from that point forward to stop people from pushing new code that violates the rules we have managed to agree on. This requires a tool that can enable individual checks one by one. Secondly, the tool can't go in and modify people's code, it can only verify that it is correct and give a warning if there is a violation. This means that what we are looking for is a validator, not a formatter. There must also be the option to disable validation for parts of the code.

I have not found any such tools out there, one that can accept the hotspot source code as is without any remarks if all checks are disabled, and then let you enable one check a time as we gradually clean up the code. The best I could find was uncrustify, but it still is a formatter, not a validator, and there are a few style rules that can't be disabled. I asked the developers of uncrustify if there was any chance of having the option for it to be a validator instead but that didn't seem to be in their interest.

As I didn't find one I started to implement my own validator a few years ago. I do have a tool that swallows most of the hotspot code as is, but there are a few issues with macro usages. You see, that is another issue we have to deal with. It's not enough to parse C++. If we want to read and format the code we need to treat our macros as part of the language as well. Hotspot has a few macros that make for real challenges for a parser :-)  I haven't yet included the option to disable verification for a block of code, coming to think of it, that could actually solve the most weird macro usages we have out there. I'll take another look at that at some point.

Until we have a tool of this kind I'm afraid it will be extremely difficult to enforce any style rules in practice. But I do agree that this is a goal worth pursuing.

Cheers,
/Jesper


> 
>> Reference:
>> [1] https://clang.llvm.org/docs/ClangFormat.html <https://clang.llvm.org/docs/ClangFormat.html>
>> Thanks,
>> --lx