8311227: Add .editorconfig so IDEs would pick up the common settings automatically: indent, trim trailing whitespace

[dbriemann]

Add an .editorconfig to define indentation, trim trailing whitespace and open curly brace position for C++ and Java.
This allows various editors to easily infer basics of the coding style.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8311227

Issue

• JDK-8311227: Add .editorconfig (Enhancement - P4) ⚠️ Title mismatch between PR and JBS.

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)
• Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23693/head:pull/23693
$ git checkout pull/23693

Update a local copy of the PR:
$ git checkout pull/23693
$ git pull https://git.openjdk.org/jdk.git pull/23693/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23693

View PR using the GUI difftool:
$ git pr show -t 23693

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23693.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @dbriemann, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user dbriemann" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

@dbriemann This change is no longer ready for integration - check the PR body for details.

[dbriemann]

/covered

[openjdk]

@dbriemann The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[bridgekeeper]

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[mlbridge]

Webrevs

• 02: Full - Incremental (c0e5ed47)
• 01: Full - Incremental (349997a8)
• 00: Full (02f923e2)

[erikj79]

Attempts at introducing any kind of code standard in OpenJDK has historically failed. It's a touchy subject to many. The bug description only really mentions trim_trailing_whitespace and since we are already enforcing this through jcheck, I don't see a problem or controversy adding that configuration here. As for the rest, I'm unsure.

The indent_brace_style isn't listed as an official property of EditorConfig (https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties). Is it actually picked up by any editor? Regardless, this isn't something that can just be snuck in.

What does indent_style = tab actually mean for makefiles? The tab character is used for defining recipe lines. Either an editor understands makefiles or it doesn't, I don't think this setting will be helpful. Also, most of our makefiles are *.gmk.

Defining indent_size for other source files is likely controversial, but if we were to go that route, then the set of cpp,hpp,c,h is not complete. Defining indent_style = space is likely ok for most kinds of sources, but again, I don't think we can just sneak this in like this.

My suggestion would be to only include the trim_trailing_whitespace line if you want to get this file in. After that you are free to start campaigning for more settings, but be prepared to work for it.

[magicus]

I think a .editorconfig file is a good idea. There is an old feature request about this... checking ... actually, I see you've found and reused that JBS bug. :-)

However, as Erik says, you need to be super careful not to try to enforce something that is not already enforced. I do support you in extending the scope of what rules we enforce, but as Erik says, prepare for an uphill battle.

I suggest you follow slavishly what the .jcheck/conf file prescribes:

[checks "whitespace"]
files=.*\.cpp|.*\.hpp|.*\.c|.*\.h|.*\.java|.*\.cc|.*\.hh|.*\.m|.*\.mm|.*\.S|.*\.md|.*\.properties|.*\.gmk|.*\.m4|.*\.ac|Makefile
ignore-tabs=.*\.gmk|Makefile

The "whitespace" check enforces, afaik, two things:

1. That there are no trailing whitespaces on a line
2. That all leading whitespaces are space (i.e., that tabs are forbidden).

The latter rule is removed by the ignore-tabs rule, since Makefile ascribes semantic differences to leading space and tabs (yeah, I know, it's braindead) and we need to allow both.

Note that no other file than what is matched by the regexp above are checked for whitespace. As a consequence, trailing whitespaces are galore in text files, xml files, you name it. So a rule that such whitespaces should be stripped would be highly disruptive, at least until these files are fixed and an enforcement rule is added to the jcheck file.

I don't know enough of the .editorconfig format to know if it is possible to express these rules exact, but if not, you better aim for a subset, rather than a superset.

[dbriemann]

Thank you both for your input. I added an editorconfig locally because because I have other defaults for C++, so my editor can switch to "JDK mode" when working in this project. I put the file into my local .gitignore but thought this might make sense to have it in the repo. Then I found the JBS issue and opened the PR to get the ball rolling.

The indent_brace_style isn't listed as an official property of EditorConfig

You are right. It seems to be only some extension proposal. I removed it.

What does indent_style = tab actually mean for makefiles? [...]

I agree, this seems unnecessary. I also removed this part.

As for the coding style (indentation), I followed the official style guides to define this.

For C++ we have https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#whitespace

which states:

Indentation levels are two columns.

and

Tabs are not allowed in code.

so the given config seemed a logical result.

For Java I referred to this https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html which states:

Four spaces should be used as the unit of indentation. The exact construction of the indentation (spaces vs. tabs) is unspecified. Tabs must be set exactly every 8 spaces (not 4).

To be honest, this is a bit unclear to me. So maybe I got it wrong in defining 4 spaces here.

I guess the question is: are these guidelines obligatory and if so why can we not define this in an editorconfig?

EDIT:
I would not see much sense in only adding the trim_trailing_whitespace option. Then vital settings are missing and I have a file that conflicts with my local one :)

[kimbarrett]

The HotSpot style guide prescribes 2 space indentation. The rest of the JDK not so much, and
counterexamples abound. For example, java.base/*/native looks to be 4 space indentation
(consistently? I only spot-checked).

[magicus]

I guess the question is: are these guidelines obligatory and if so why can we not define this in an editorconfig?

No, they are not. That's kind of the crux of the problem.

• The Hotspot guidelines are followed to a great extent, but I'm pretty sure they also say something along the lines of "if old code does not adhere to this, do not change everything blindly", which is exactly what an editor would do.
• Hotspot is not the only C++ code in the JDK; and the rest of the native libraries has nothing even close to the Hotspot guidelines. Furthermore, there are imported 3rd party sources in the code base that are exempt from any kind of requirements.
• The Java guidelines, how official they may look, is not considered really applicable to the JDK code. I guess most Java code uses four spaces of indentation, but it is not a clear rule.

[magicus]

I would not see much sense in only adding the trim_trailing_whitespace option. Then vital settings are missing and I have a file that conflicts with my local one :)

I apologize if such a solution would cause a conflict for your setup, but I seriously think that this is the only way forward. We can setup an editorconfig file that matches what jcheck already checks for, but not anything more. Then we can work on, going forward, to increase the official rules about code. That would allow jcheck to test for more aspects, and an editorconfig to be more explicit.

[kimbarrett]

As for the coding style (indentation), I followed the official style guides to define this.

For C++ we have https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#whitespace

which states:

Indentation levels are two columns.

The HotSpot Style Guide applies to HotSpot native code. It doesn't apply to native code in the rest of the JDK.
(For one thing, HotSpot is (almost?) entirely C++, while much of the native code in the rest of the JDK is C.
And even a casual perusal will find large swaths of HSG violations in non-HotSpot code.)

[erikj79]

As for the coding style (indentation), I followed the official style guides to define this.
For C++ we have https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#whitespace
which states:

Indentation levels are two columns.

The HotSpot Style Guide applies to HotSpot native code. It doesn't apply to native code in the rest of the JDK. (For one thing, HotSpot is (almost?) entirely C++, while much of the native code in the rest of the JDK is C. And even a casual perusal will find large swaths of HSG violations in non-HotSpot code.)

We could consider adding a separate .editorconfig file in src/hotspot with these C++ rules.

[dbriemann]

I see. I didn't understand that the hotspot style guide is only for the hotspot part before.. seems obvious now :)

I like @erikj79 idea and have adapted the PR to this. It now defines trim_trailing_whitespace globally and indentation of 2 spaces for c/c++ hotspot code only.

[erikj79]

I think this looks acceptable.

[erikj79]

/reviewers 2 reviewer

[openjdk]

@erikj79
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[magicus]

There are no C files in Hotspot, nor should there ever be. (There are a handful of .h files, though, for interop purposes)

[dbriemann]

Yes I saw that there are no C files but added it for symmetry with the H files. I don't think it hurts here but I can also remove it. Do you think it should be removed?

[magicus]

I think that is rather up to the Hotspot folks to have an opinion on.

[dbriemann]

/label add hotspot

[openjdk]

@dbriemann
The hotspot label was successfully added.

[TheRealMDoerr]

It'd be nice if we could have a default style for .java files in the future, too. This looks good to me for the time being.

[magicus]

I still don't think the .editorconfig as suggested here is a good idea. It directly conflicts with the existing logic in jcheck.

Understand me right -- I am all in favor of tightening the structure of our code base. But we can't do that by introducing an .editorconfig that does not match what is currently enforced by jcheck or is the current standard of the code base. Instead, we need to tighten the rules bit by bit, getting buy-in for tighter rules, and ensuring we update and fix all old files.

I have published an alternative implementation of this issue at #24448. That version of .editorconfig has a 1-to-1 correspondence to what is checked by jcheck.

[dbriemann]

I have to respectfully disagree with your assessment for the following reasons:

• The editorconfig as defined in this PR follows the hotspot style guide and the indentation settings only apply to hotspot code.
• How does it conflict with the jcheck rules? The rules defined in this editorconfig are stricter than the jcheck rules. So jcheck could not find issues after the editorconfig rules were applied.
• Providing an editorconfig as you proposed in 8311227: Add .editorconfig #24448 is not a good idea in my opinion.
  Only defining whitespace trimming will not provide real benefit to anyone. However the existing editorconfig file would conflict with locally defined ones and cause problems for developers using these files to define project specific formatting.
  So no editorconfig would be better than one representing only a small part of the core formatting rules.

I also would ask about jcheck. Where and how is it used. The only information I found are:
https://openjdk.org/projects/code-tools/jcheck/ and the config file in jdk/.jcheck.

Thanks

[magicus]

• How does it conflict with the jcheck rules? The rules defined in this editorconfig are stricter than the jcheck rules. So jcheck could not find issues after the editorconfig rules were applied.

No, it's not. The rules you propose here are stricter. You say:

[*]
trim_trailing_whitespace = true

but there is not nor have ever been such a rule for all text files in the JDK repo. This would trigger an enormous amount of spurious changes. In contrast, my suggested PR applies this only to the subset of files where we do in fact have a rule of no trailing whitespaces.

I also would ask about jcheck. Where and how is it used. The only information I found are:
https://openjdk.org/projects/code-tools/jcheck/ and the config file in jdk/.jcheck.

jcheck is run automatically by the Skara bots on all PRs. If jcheck reports an error (that is, a violation of enforced style rules), the PR will not be possible to integrate.

Users can also run jcheck locally using the git skara jcheck command, if they have the Skara git tools installed.

[magicus]

• The editorconfig as defined in this PR follows the hotspot style guide and the indentation settings only apply to hotspot code.

Yes, you have a good point there. I included your src/hotspot/.editorconfig in my PR.

[dbriemann]

I see. So you were addressing the file types specifically. I thought you had an issue with the indentation. So since you already included the indentation in the other PR I will just approve yours and close mine.

Thanks for the explanation of jcheck.

[dbriemann]

Closing in favor of #24448

[magicus]

I thought you had an issue with the indentation.

I had an issue with indentation in the original PR, where you wanted to apply it to all files and not just Hotspot, but not the current version.