Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper #5232

Closed
wants to merge 1 commit into from

Conversation

@gredler
Copy link
Contributor

@gredler gredler commented Aug 23, 2021

During the recent JEP 381 removal of Solaris code, a few Solaris-specific constants and private methods were left behind in sun.font.TrueTypeGlyphMapper. This PR removes these unused odds and ends.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5232/head:pull/5232
$ git checkout pull/5232

Update a local copy of the PR:
$ git checkout pull/5232
$ git pull https://git.openjdk.java.net/jdk pull/5232/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5232

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5232.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 23, 2021

👋 Welcome back gredler! 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
Copy link

@openjdk openjdk bot commented Aug 23, 2021

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

  • 2d

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.

@openjdk openjdk bot added the 2d label Aug 23, 2021
@gredler
Copy link
Contributor Author

@gredler gredler commented Aug 23, 2021

It looks like the commit message must reference an issue, so I've submitted an issue via https://bugreport.java.com/. The internal review ID is 9071267. I can update the commit message once the issue has been reviewed and assigned an actual issue number.

@prrace
Copy link
Contributor

@prrace prrace commented Aug 25, 2021

/issue JDK-8272879

@openjdk
Copy link

@openjdk openjdk bot commented Aug 25, 2021

@prrace Only the author (@gredler) is allowed to issue the /issue command.

@gredler
Copy link
Contributor Author

@gredler gredler commented Aug 25, 2021

/issue JDK-8272879

@openjdk
Copy link

@openjdk openjdk bot commented Aug 25, 2021

@gredler The issue 8272879 was not found in the JDK project - make sure you have entered it correctly.
As there were validation problems, no additional issues will be added to the list of solved issues.

@gredler
Copy link
Contributor Author

@gredler gredler commented Aug 25, 2021

/issue JDK-8272878

@openjdk openjdk bot changed the title JEP 381 cleanup: Remove unused Solaris code 8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper Aug 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 25, 2021

@gredler The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

@openjdk openjdk bot added the rfr label Aug 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 25, 2021

Webrevs

prrace
prrace approved these changes Aug 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 25, 2021

@gredler 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:

8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper

Reviewed-by: prr, jdv

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 119 new commits pushed to the master branch:

  • c640fe4: 7188098: TEST_BUG: closed/javax/sound/midi/Synthesizer/Receiver/bug6186488.java fails
  • cec6c06: 8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
  • 14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
  • d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils
  • 23fa0dc: 8272905: Consolidate discovered lists processing
  • ff4018b: 8268148: unchecked warnings handle ? and ? extends Object differently
  • 8c37909: 8273234: extended 'for' with expression of type tvar causes the compiler to crash
  • 28ba78e: 8244675: assert(IncrementalInline || (_late_inlines.length() == 0 && !has_mh_late_inlines()))
  • d05494f: 8266239: Some duplicated javac command-line options have repeated effect
  • 93eec9a: 8272776: NullPointerException not reported
  • ... and 109 more: https://git.openjdk.java.net/jdk/compare/9bc023220fbbb0b6ea1ed1a0ca2aa3848764e8cd...master

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 (@prrace, @jayathirthrao) 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).

@openjdk openjdk bot added the ready label Aug 25, 2021
@gredler
Copy link
Contributor Author

@gredler gredler commented Aug 25, 2021

/integrate

@openjdk openjdk bot added the sponsor label Aug 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 25, 2021

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

@prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Aug 26, 2021

As per JEP 381, comments pointing to Solaris also need to be looked into or purged. There seem to be around 96 mention of Solaris still in java_desktop module...Can it be looked into in this PR?

@gredler
Copy link
Contributor Author

@gredler gredler commented Aug 30, 2021

I had initially wanted to keep this PR focused on a small, self-contained, fairly obvious change, since this is my first contribution to the project -- but I'm open to expanding the scope if I can get some direction.

It can be argued that many of the comments which mention Solaris still provide useful background information, e.g.:

* 2) Some font configuration files on Solaris reference two versions

* suffice in Solaris UTF-8 locales where a single composite strike may be

/* This test is too stringent for Arial on Solaris (and perhaps

There are some other comments that could more obviously be fixed without issues:

* A default implementation is given for Linux, Solaris and Windows.

@prsadhuk @prrace How would you like me to proceed?

@prrace
Copy link
Contributor

@prrace prrace commented Aug 30, 2021

I'm with Daniel on this one. I'd have to think about how to update those comments.
It isn't just an obvious case of "oh it mentions Solaris, I'll delete the whole thing".
So let's keep this PR focused.

@prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Aug 31, 2021

I do not have any objection to this looking into comments in future and not in this PR.
What about the code that has solaris in it?
https://github.com/openjdk/jdk/blob/master/src/java.desktop/unix/classes/sun/awt/X11/XScrollbarPeer.java#L37
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/native/liblcms/lcms2.h#L534 [it might need change in upstream though]

@gredler
Copy link
Contributor Author

@gredler gredler commented Sep 3, 2021

I think LittleCMS explicitly supports Solaris, so I doubt a patch removing Solaris support would be accepted upstream.

It does look like XScrollbarPeer should be cleaned up, but should it be done as part of this PR? Let me know either way.

I'm also looking for a sponsor for this PR, would either @prrace or @prsadhuk have some time to spare? Thanks!

@prrace
Copy link
Contributor

@prrace prrace commented Sep 3, 2021

Completely out of scope by a country mile to get anywhere near the LCMS support for Solaris.
In fact a really bad idea. Current JDK does not support Solaris but current LCMS may want to support older Solaris

I will sponsor this if it is ready. The submitter needs to type /integrate and only after that does /sponsor do anything.

@gredler
Copy link
Contributor Author

@gredler gredler commented Sep 3, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

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

@gredler
Copy link
Contributor Author

@gredler gredler commented Sep 3, 2021

@prsadhuk I will raise a separate PR in the coming days to address the XScrollbarPeer cleanup.

@jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Sep 6, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Sep 6, 2021

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

  • b4e5b28: 8273221: Guard GCIdMark against nested calls
  • 4d25e6f: 8273335: compiler/blackhole tests should not run with interpreter-only VMs
  • c640fe4: 7188098: TEST_BUG: closed/javax/sound/midi/Synthesizer/Receiver/bug6186488.java fails
  • cec6c06: 8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
  • 14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
  • d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils
  • 23fa0dc: 8272905: Consolidate discovered lists processing
  • ff4018b: 8268148: unchecked warnings handle ? and ? extends Object differently
  • 8c37909: 8273234: extended 'for' with expression of type tvar causes the compiler to crash
  • 28ba78e: 8244675: assert(IncrementalInline || (_late_inlines.length() == 0 && !has_mh_late_inlines()))
  • ... and 111 more: https://git.openjdk.java.net/jdk/compare/9bc023220fbbb0b6ea1ed1a0ca2aa3848764e8cd...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 6, 2021

@jayathirthrao @gredler Pushed as commit 70ed6c5.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants