Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Thu, 22 Oct 2020 08:48:20 GMT, Serguei Spitsyn wrote:
>> File test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001.java has
>> this change:
>>
>> for (int i = 0; i < threads.length; i++) {
>> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.kill +
>> threads[i] + " " +
>> - DEBUGGEE_EXCEPTIONS
>> + "[" + i + "]",
>> - "killed");
>> +DEBUGGEE_EXCEPTIONS + "[" + i + "]",
>> +"killed");
>> }
>> I think, the second line "killed"); has to be aligned with the previous one.
>> Also, I feels like this change makes the code to be less readable:
>> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.eval +
>> DEBUGGEE_RESULT,
>> - DEBUGGEE_RESULT + " =");
>> +DEBUGGEE_RESULT + " =");
>
> Hi Igor,
>
> Overall, it is great. Your formatting tool seems to be AI.
> This update fixes a lot of formatting issues.
> I have no more comments so far.
Regarding the "killed" formatting, I think the reason for it is the 8 space
"line continuation rule". The 2nd line is a actually a line continuation of a
line continuation, so it gets indented and extra 16 spaces. The 3rd line is
just a single line continuation, so gets indented just an extra 8. I think what
would help to clean this up is to move the first argument onto a newline, which
would just be indented an extra 8. That way it won't be broken up over multiple
lines, and will also be inline with the "killed" argument.
For the 2nd case above, I also think this is less readable than the original. I
personally like it if you align all arguments with the first one, even if it
leads to a non-standard indentation. If you want subsequent arguments to use
the 8 space line continuation rule, then I suggest whenever arguments are on
multiple lines that you always place the first argument on a new line so all
arguments are aligned together.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Thu, 22 Oct 2020 08:03:15 GMT, Serguei Spitsyn wrote:
>> due to the same reasons in the case w/ `fields001`, these lines have 3 unit
>> indentation, 1st for `hc001` class, 2nd for `testInvalidCommands` method,
>> 3rd for `invClassNames` array initialization, so they have 3x4 = 12 spaces.
>
> File test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001.java has this
> change:
>
> for (int i = 0; i < threads.length; i++) {
> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.kill +
> threads[i] + " " +
> - DEBUGGEE_EXCEPTIONS +
> "[" + i + "]",
> - "killed");
> +DEBUGGEE_EXCEPTIONS + "[" + i + "]",
> +"killed");
> }
> I think, the second line "killed"); has to be aligned with the previous one.
> Also, I feels like this change makes the code to be less readable:
> reply = jdb.receiveReplyForWithMessageWait(JdbCommand.eval +
> DEBUGGEE_RESULT,
> - DEBUGGEE_RESULT + " =");
> +DEBUGGEE_RESULT + " =");
Hi Igor,
Overall, it is great. Your formatting tool seems to be AI.
This update fixes a lot of formatting issues.
I have no more comments so far.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 20:57:03 GMT, Igor Ignatyev wrote:
>> test/hotspot/jtreg/vmTestbase/nsk/jdb/hidden_class/hc001/hc001.java line 323:
>>
>>> 321: "xx.yyy/0x111/0x222",
>>> 322: "xx./0x111.0x222",
>>> 323: "xx.yyy.zzz/"
>>
>> Why are these indented 8 instead of 4?
>
> due to the same reasons in the case w/ `fields001`, these lines have 3 unit
> indentation, 1st for `hc001` class, 2nd for `testInvalidCommands` method, 3rd
> for `invClassNames` array initialization, so they have 3x4 = 12 spaces.
File test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001.java has this
change:
for (int i = 0; i < threads.length; i++) {
reply = jdb.receiveReplyForWithMessageWait(JdbCommand.kill +
threads[i] + " " +
- DEBUGGEE_EXCEPTIONS +
"[" + i + "]",
- "killed");
+DEBUGGEE_EXCEPTIONS + "[" + i + "]",
+"killed");
}
I think, the second line "killed"); has to be aligned with the previous one.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Wed, 21 Oct 2020 21:10:54 GMT, Igor Ignatyev wrote: >> Do you plan on leaving these indented 8 instead of 4 because that's what >> IDEA defaults to? Other editors, like emacs, will want to indent them 4, and >> will do so often do so automatically just by editing the line, depending on >> whether you type any character that forces reformatting of indentation. >> >> Do we have coding guidelines that say to indent them 8? > >> Do we have coding guidelines that say to indent them 8? > > old "official" guidelines don't tell much about it, @aioobe's guidelines > don't special-case array initiation, so it is covered by: >> A continuation line should be indented in one of the following four ways >> Variant 1: With 8 extra spaces relative to the indentation of the previous >> line. >> Variant 2: With 8 extra spaces relative to the starting column of the >> wrapped expression. >> Variant 3: Aligned with previous sibling expression (as long as it is clear >> that it’s a continuation line) >> Variant 4: Aligned with previous method call in a chained expression. > > which will suggest that 8 is the right indent here. > > google's code-style says that "array initializer may _optionally_ be > formatted as if it were a "block-like construct.", which would mean 4, but > note "may" and "_optionally_". personally I'd prefer consistency here, but if > you think it will make the lives of emacs-lovers miserable, although I'm sure > you can configure emacs to treat array-init as line continuation, and not as > a block, I can revert that. I don't consider this to be a continuation line. It just looks too much like a statement block. My other concern is too much seemingly arbitrary reformatting. I really don't like the idea of reformatting an entire file based on some formatter's notion of what is best or correct, leading to unnecessary reformatting like this one. It muddles the history. The exception would be for a file that is hopelessly formatted incorrectly, like using 2-char indentation instead of 4 in java, or 4 char instead of 2 in C. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Wed, 21 Oct 2020 20:12:51 GMT, Chris Plummer wrote: > Do we have coding guidelines that say to indent them 8? old "official" guidelines don't tell much about it, @aioobe's guidelines don't special-case array initiation, so it is covered by: > A continuation line should be indented in one of the following four ways > Variant 1: With 8 extra spaces relative to the indentation of the previous > line. > Variant 2: With 8 extra spaces relative to the starting column of the wrapped > expression. > Variant 3: Aligned with previous sibling expression (as long as it is clear > that it’s a continuation line) > Variant 4: Aligned with previous method call in a chained expression. which will suggest that 8 is the right indent here. google's code-style says that "array initializer may _optionally_ be formatted as if it were a "block-like construct.", which would mean 4, but note "may" and "_optionally_". personally I'd prefer consistency here, but if you think it will make the lives of emacs-lovers miserable, although I'm sure you can configure emacs to treat array-init as line continuation, and not as a block, I can revert that. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Tue, 20 Oct 2020 00:21:14 GMT, Igor Ignatyev wrote: >> I don't follow. You seem to have added an extra 4 (after having already >> indented 4 extra spaces) simply because it is an array initialization. Why >> is the indentation any different than it would be for something like a "for" >> loop block. The body of the block is indented 4 spaces more than the "for" >> statement. For the static initialization the body of that "static" block >> should be indented 4 more than the "static" statement. I don't see anything >> in the style guide that would indicate otherwise. > > oh, sorry, it appears I was trying to explain the new code while looking at > the old one... > > full disclaimer, which I thought was obvious, the vast majority of the > changes here aren't done by me, they are done by auto-formatter, or more > precisely by IntelliJ IDEA's formatter. IDEA doesn't consider array > initialization as a new block, and what's actually at play here is an indent > for statement continuation (which is 2x of regular indent). Do you plan on leaving these indented 8 instead of 4 because that's what IDEA defaults to? Other editors, like emacs, will want to indent them 4, and will do so often do so automatically just by editing the line, depending on whether you type any character that forces reformatting of indentation. Do we have coding guidelines that say to indent them 8? - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Tue, 20 Oct 2020 01:13:33 GMT, Chris Plummer wrote: >> although horizontal alignment (of variable names, initialization, >> expressions, etc) seems to somewhat improve >> readability, it almost always associated with a higher maintenance cost, and >> the current consensus is not to do that. >> from 'Horizontal Whitespace' section of the same >> [guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-whitespace): >>> In variable declarations it is not recommended to align types and variables. >> *Motivation* >> The improvement in readability when aligning variable names is negligible >> compared to the efforts needed to keep them >> aligned as the code evolves. Realigning all variables when one of the types >> change also causes unnecessarily >> complicated patches to review. other java code guidelines either discourage >> horizontal alignment or consider it >> optional and provide the same motivation as above to why it's better not to >> have it. > > While in general I agree that gratuitous aligning of variables is not > desirable, this is a special case where it adds > value (enough that I think it's worth doing). The value here is due to the > fact that the variables are building on each > other. When they are aligned it's easier to scan for the variable (and its > value) that another variable builds on. You > can keep your change in place if you feel that consistency and maintainabilty > is more important here than readability. Although I don't think that the test code has the same frequency of changes as the product code, I'd still prefer consistency and maintainability over some small penalty (which is IMHO negligible) in readability. frankly, it's also partially motivated by my laziness as I don't want to go over all these files again and examine each and every initialization for possible (scanty) readability improvements from horizontal alignment. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Tue, 20 Oct 2020 00:04:16 GMT, Igor Ignatyev wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002.java >> line 87: >> >>> 85: static final String DEBUGGEE_CLASS = TEST_CLASS + "a"; >>> 86: static final String FIRST_BREAK = DEBUGGEE_CLASS + ".main"; >>> 87: static final String LAST_BREAK = DEBUGGEE_CLASS + ".lastBreak"; >> >> I'm not so sure I'd consider this change an improvement. Maybe remove some >> of the extra spaces but keep the alignment. > > although horizontal alignment (of variable names, initialization, > expressions, etc) seems to somewhat improve > readability, it almost always associated with a higher maintenance cost, and > the current consensus is not to do that. > from 'Horizontal Whitespace' section of the same > [guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-whitespace): >> In variable declarations it is not recommended to align types and variables. > *Motivation* > The improvement in readability when aligning variable names is negligible > compared to the efforts needed to keep them > aligned as the code evolves. Realigning all variables when one of the types > change also causes unnecessarily > complicated patches to review. other java code guidelines either discourage > horizontal alignment or consider it > optional and provide the same motivation as above to why it's better not to > have it. While in general I agree that gratuitous aligning of variables is not desirable, this is a special case where it adds value (enough that I think it's worth doing). The value here is due to the fact that the variables are building on each other. When they are aligned it's easier to scan for the variable (and its value) that another variable builds on. You can keep your change in place if you feel that consistency and maintainabilty is more important here than readability. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 23:56:00 GMT, Chris Plummer wrote: >> b/c you get indentation unit for each block, 1st block is `fields001` class >> definition, next block is the array >> initializations (start at L#86 and L#99 for `checkedFields1` and >> `checkedFields2` respecitively) > > I don't follow. You seem to have added an extra 4 (after having already > indented 4 extra spaces) simply because it is > an array initialization. Why is the indentation any different than it would > be for something like a "for" loop block. > The body of the block is indented 4 spaces more than the "for" statement. > For the static initialization the body of > that "static" block should be indented 4 more than the "static" statement. I > don't see anything in the style guide that > would indicate otherwise. oh, sorry, it appears I was trying to explain the new code while looking at the old one... full disclaimer, which I thought was obvious, the vast majority of the changes here aren't done by me, they are done by auto-formatter, or more precisely by IntelliJ IDEA's formatter. IDEA doesn't consider array initialization as a new block, and what's actually at play here is an indent for statement continuation (which is 2x of regular indent). - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 18:37:58 GMT, Chris Plummer wrote: >> Igor Ignatyev has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright years > > test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002.java > line 87: > >> 85: static final String DEBUGGEE_CLASS = TEST_CLASS + "a"; >> 86: static final String FIRST_BREAK = DEBUGGEE_CLASS + ".main"; >> 87: static final String LAST_BREAK = DEBUGGEE_CLASS + ".lastBreak"; > > I'm not so sure I'd consider this change an improvement. Maybe remove some of > the extra spaces but keep the alignment. although horizontal alignment (of variable names, initialization, expressions, etc) seems to somewhat improve readability, it almost always associated with a higher maintenance cost, and the current consensus is not to do that. from 'Horizontal Whitespace' section of the same [guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-whitespace): > In variable declarations it is not recommended to align types and variables. *Motivation* The improvement in readability when aligning variable names is negligible compared to the efforts needed to keep them aligned as the code evolves. Realigning all variables when one of the types change also causes unnecessarily complicated patches to review. other java code guidelines either discourage horizontal alignment or consider it optional and provide the same motivation as above to why it's better not to have it. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 20:55:01 GMT, Igor Ignatyev wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jdb/fields/fields001/fields001.java line >> 108: >> >>> 106: "ii_aa", "oi_aa", >>> 107: "ii_aaa", "oi_aaa" >>> 108: }; >> >> Why are these indented 8 instead of 4? > > b/c you get indentation unit for each block, 1st block is `fields001` class > definition, next block is the array > initializations (start at L#86 and L#99 for `checkedFields1` and > `checkedFields2` respecitively) I don't follow. You seem to have added an extra 4 (after having already indented 4 extra spaces) simply because it is an array initialization. Why is the indentation any different than it would be for something like a "for" loop block. The body of the block is indented 4 spaces more than the "for" statement. For the static initialization the body of that "static" block should be indented 4 more than the "static" statement. I don't see anything in the style guide that would indicate otherwise. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 20:49:14 GMT, Igor Ignatyev wrote:
>> test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002a.java
>> line 42:
>>
>>> 40: }
>>> 41:
>>> 42: public int runIt(String[] args, PrintStream out) {
>>
>> Is there a style guide that says this is the preferred way to declare an
>> array type? I count 3700 occurrences of
>> "String args[]" in ours tests.
>
> yes, there is, e.g. [Java Style
> Guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-variable-declarations)
> by Andreas
> Lundblad:
>> Square brackets of arrays should be at the type (String[] args) and not on
>> the variable (String args[]).
>
> although @aioobe's guidelines have been accepted (yet?) to the openjdk guide
> (see openjdk/guide#14), this particular
> item is agreed on and accepted almost unanimously in the industry.
ok
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 18:51:15 GMT, Chris Plummer wrote: >> Igor Ignatyev has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright years > > test/hotspot/jtreg/vmTestbase/nsk/jdb/fields/fields001/fields001.java line > 108: > >> 106: "ii_aa", "oi_aa", >> 107: "ii_aaa", "oi_aaa" >> 108: }; > > Why are these indented 8 instead of 4? b/c you get indentation unit for each block, 1st block is `fields001` class definition, next block is the array initializations (start at L#86 and L#99 for `checkedFields1` and `checkedFields2` respecitively) > test/hotspot/jtreg/vmTestbase/nsk/jdb/hidden_class/hc001/hc001.java line 323: > >> 321: "xx.yyy/0x111/0x222", >> 322: "xx./0x111.0x222", >> 323: "xx.yyy.zzz/" > > Why are these indented 8 instead of 4? due to the same reasons in the case w/ `fields001`, these lines have 3 unit indentation, 1st for `hc001` class, 2nd for `testInvalidCommands` method, 3rd for `invClassNames` array initialization, so they have 3x4 = 12 spaces. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 18:44:56 GMT, Chris Plummer wrote:
>> Igor Ignatyev has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update copyright years
>
> test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002a.java
> line 42:
>
>> 40: }
>> 41:
>> 42: public int runIt(String[] args, PrintStream out) {
>
> Is there a style guide that says this is the preferred way to declare an
> array type? I count 3700 occurrences of
> "String args[]" in ours tests.
yes, there is, e.g. [Java Style
Guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-variable-declarations)
by Andreas
Lundblad:
> Square brackets of arrays should be at the type (String[] args) and not on
> the variable (String args[]).
although @aioobe's guidelines have been accepted (yet?) to the openjdk guide
(see openjdk/guide#14), this particular
item is agreed on and accepted almost unanimously in the industry.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Thu, 15 Oct 2020 23:25:25 GMT, Igor Ignatyev wrote:
>> Hi all,
>>
>> could you please review this patch which reformats/cleans up the code of
>> vmTestbase/nsk/jdb tests?
>>
>> the part of this patch is a spinoff from #350.
>>
>> testing:
>> * [x] `vmTestbase/nsk/jdb` tests on `linux-x64-debug`
>
> Igor Ignatyev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyright years
I've gotten about halfway through. I'll wait for your responses before doing
the rest since I don't want to keep
tagging the same issues.
test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002.java
line 87:
> 85: static final String DEBUGGEE_CLASS = TEST_CLASS + "a";
> 86: static final String FIRST_BREAK = DEBUGGEE_CLASS + ".main";
> 87: static final String LAST_BREAK = DEBUGGEE_CLASS + ".lastBreak";
I'm not so sure I'd consider this change an improvement. Maybe remove some of
the extra spaces but keep the alignment.
test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002a.java
line 42:
> 40: }
> 41:
> 42: public int runIt(String[] args, PrintStream out) {
Is there a style guide that says this is the preferred way to declare an array
type? I count 3700 occurrences of
"String args[]" in ours tests.
test/hotspot/jtreg/vmTestbase/nsk/jdb/fields/fields001/fields001.java line 108:
> 106: "ii_aa", "oi_aa",
> 107: "ii_aaa", "oi_aaa"
> 108: };
Why are these indented 8 instead of 4?
test/hotspot/jtreg/vmTestbase/nsk/jdb/hidden_class/hc001/hc001.java line 323:
> 321: "xx.yyy/0x111/0x222",
> 322: "xx./0x111.0x222",
> 323: "xx.yyy.zzz/"
Why are these indented 8 instead of 4?
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
> Hi all, > > could you please review this patch which reformats/cleans up the code of > vmTestbase/nsk/jdb tests? > > the part of this patch is a spinoff from #350. > > testing: > * [x] `vmTestbase/nsk/jdb` tests on `linux-x64-debug` Igor Ignatyev has updated the pull request incrementally with one additional commit since the last revision: update copyright years - Changes: - all: https://git.openjdk.java.net/jdk/pull/689/files - new: https://git.openjdk.java.net/jdk/pull/689/files/1b6fa98f..e43129d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=689=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=689=00-01 Stats: 64 lines in 64 files changed: 0 ins; 0 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/689.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/689/head:pull/689 PR: https://git.openjdk.java.net/jdk/pull/689
