8296420: javac has long lines in its command-line help #13329

archiecobbs · 2023-04-04T16:16:51Z

The issue states that the output of javac --help and javac --help-extra should not overflow 80 columns.

This patch updates javac.properties to add newlines, etc. to address this issue, and adds a regression test.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8296420: javac has long lines in its command-line help

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer) ⚠️ Review applies to 37ccfd06

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13329

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-04-04T16:18:36Z

👋 Welcome back archiecobbs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-04-04T16:20:26Z

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

compiler

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.

mlbridge · 2023-04-04T16:24:19Z

Webrevs

jonathan-gibbons · 2023-04-04T17:32:20Z

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties

 javac.opt.Werror=\
    Terminate compilation if warnings occur
 javac.opt.A=\
    Options to pass to annotation processors
 javac.opt.implicit=\
-    Specify whether or not to generate class files for implicitly referenced files
+    Specify whether or not to generate class files\n\


There's been a lot of discussion (elsewhere) of late regarding the phrase whether or not.
Generally, the consensus seems to be that the correct form in cases like this is just whether and that whether or not idiomatically means something else. With that in mind, I'd suggest dropping or not and then see how that affects the needs for any line-wrapping.

Nice - and now it fits in one line :)

Fixed in cfc2c3d.

jonathan-gibbons · 2023-04-04T17:34:27Z

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties

 javac.opt.preview=\
-    Enable preview language features. To be used in conjunction with either -source or --release.
+    Enable preview language features. To be used in conjunction\n\


I'd consider breaking "early", after the initial period.

Fixed in b43a1ca.

jonathan-gibbons · 2023-04-04T17:36:56Z

test/langtools/tools/javac/options/HelpOutputColumnWidthTest.java

+        final String tooLongLine = log.stream()
+          .filter(line -> line.length() > MAX_COLUMNS)
+          .findFirst()
+          .orElse(null);
+        if (tooLongLine != null)
+            throw new Exception("output line too long: \"" + tooLongLine.trim() + "\"");
+    }


This is good. It would be even better if you removed findFirst and then reported all the lines if the resulting list is non-empty.

Nice idea. Fixed in 2137122.

jonathan-gibbons

Nice work; nice test!

One optional suggestion to improve the test.

openjdk · 2023-04-04T17:38:51Z

@archiecobbs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8296420: javac has long lines in its command-line help

Reviewed-by: jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

94a05e0: 8305599: (fc) Temporarily problem-list java/nio/channels/{AsyncCloseAndInterrupt.java, FileChannel/Transfer.java}
7c65048: 8305343: BigDecimal.fractionOnly() erroneously returns true for large scale value
dd59471: 8304846: Provide a shared utility to dump generated classes defined via Lookup API
2ee4245: 8305509: C1 fails "assert(k != nullptr) failed: illegal use of unloaded klass"
7baccd9: 8303260: (fc) FileChannel::transferFrom should support position > size()
3ef834f: 8298619: java/io/File/GetXSpace.java is failing

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jonathan-gibbons) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

archiecobbs · 2023-04-04T18:30:54Z

/integrate

openjdk · 2023-04-04T18:32:48Z

@archiecobbs
Your change (at version 2137122) is now ready to be sponsored by a Committer.

vicente-romero-oracle · 2023-04-13T15:06:45Z

/sponsor

openjdk · 2023-04-13T15:09:07Z

Going to push as commit e0620b8.
Since your change was applied there have been 99 commits pushed to the master branch:

6b9b7d1: 8304350: Font.getStringBounds calculates wrong width for TextAttribute.TRACKING other than 0.0
2060c8e: 8305688: jdk build --with-memory-size=1024 broken by JDK-8305100
646b666: 8305900: Use loopback IP addresses in security policy files of httpclient tests
fb9a29d: 8305414: gtest/NMTGtests.java is failing various sub-tests
e846a1d: 8304450: [vectorapi] Refactor VectorShuffle implementation
3f36dd8: 8305529: DefaultProxySelector.select(URI) in certain cases returns a List with null element
425ef06: 8303923: ZipOutStream::putEntry should include an apiNote to indicate that the STORED compression method should be used when writing directory entries
2bbbff2: 8305858: Resolve multiple definition of 'handleSocketError' when statically linking with JDK native libraries
bc15163: 8304834: Fix wrapper insertion in TestScaffold.parseArgs(String args[])
19380d7: 8305324: C2: Wrong execution of vectorizing Interger.reverseBytes
... and 89 more: https://git.openjdk.org/jdk/compare/c59411929ddbf5fdc51ccc9d7508cfceeabc58c1...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-13T15:09:27Z

@vicente-romero-oracle @archiecobbs Pushed as commit e0620b8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Restrict javac command line help output to 80 columns.

37ccfd0

openjdk bot added the rfr Pull request is ready for review label Apr 4, 2023

openjdk bot added the compiler compiler-dev@openjdk.org label Apr 4, 2023

jonathan-gibbons reviewed Apr 4, 2023

View reviewed changes

jonathan-gibbons approved these changes Apr 4, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 4, 2023

archiecobbs added 3 commits April 4, 2023 12:48

Replace "whether or not" with just "whether".

cfc2c3d

Relocate help text split point to just after a period.

b43a1ca

Show all offending lines in exception message, not just the first.

2137122

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 4, 2023

openjdk bot added the integrated Pull request has been integrated label Apr 13, 2023

openjdk bot closed this Apr 13, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8296420: javac has long lines in its command-line help #13329

8296420: javac has long lines in its command-line help #13329

archiecobbs commented Apr 4, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 4, 2023

openjdk bot commented Apr 4, 2023

mlbridge bot commented Apr 4, 2023 •

edited

Loading

jonathan-gibbons Apr 4, 2023 •

edited

Loading

archiecobbs Apr 4, 2023

jonathan-gibbons Apr 4, 2023

archiecobbs Apr 4, 2023

jonathan-gibbons Apr 4, 2023 •

edited

Loading

archiecobbs Apr 4, 2023

jonathan-gibbons left a comment

openjdk bot commented Apr 4, 2023 •

edited

Loading

archiecobbs commented Apr 4, 2023

openjdk bot commented Apr 4, 2023

vicente-romero-oracle commented Apr 13, 2023

openjdk bot commented Apr 13, 2023

openjdk bot commented Apr 13, 2023

8296420: javac has long lines in its command-line help #13329

8296420: javac has long lines in its command-line help #13329

Conversation

archiecobbs commented Apr 4, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 4, 2023

openjdk bot commented Apr 4, 2023

mlbridge bot commented Apr 4, 2023 • edited Loading

Webrevs

jonathan-gibbons Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

archiecobbs Apr 4, 2023

Choose a reason for hiding this comment

jonathan-gibbons Apr 4, 2023

Choose a reason for hiding this comment

archiecobbs Apr 4, 2023

Choose a reason for hiding this comment

jonathan-gibbons Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

archiecobbs Apr 4, 2023

Choose a reason for hiding this comment

jonathan-gibbons left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 4, 2023 • edited Loading

archiecobbs commented Apr 4, 2023

openjdk bot commented Apr 4, 2023

vicente-romero-oracle commented Apr 13, 2023

openjdk bot commented Apr 13, 2023

openjdk bot commented Apr 13, 2023

archiecobbs commented Apr 4, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 4, 2023 •

edited

Loading

jonathan-gibbons Apr 4, 2023 •

edited

Loading

jonathan-gibbons Apr 4, 2023 •

edited

Loading

openjdk bot commented Apr 4, 2023 •

edited

Loading