8382375: Remove obsolete options from CLDRConverter#30797
8382375: Remove obsolete options from CLDRConverter#30797naotoj wants to merge 3 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
|
@naotoj 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: 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 142 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
openjdk
Bot
added
build
Webrevs
|
| formatter.format("\\u%04x", (int)aChar); | ||
| } else { | ||
| if (specialSaveChars.indexOf(aChar) != -1) { | ||
| if (aChar == 0x0022) { |
There was a problem hiding this comment.
What about using the corresponding char literal ('"') to make it easier to understand what this is escaping?
| if (aChar == 0x0022) { | |
| if (aChar == '"') { |
(This is only a suggestion; I am not an OpenJDK member, feel free to ignore this comment.)
There was a problem hiding this comment.
Thanks. I added a comment explaining it is for ASCII quotation marks
There was a problem hiding this comment.
Thanks, I think it would have been better to replace the 0x0022 with '"' (which would be functionality identical), but I assume you have your reasons why you did not want to do that.
As side note: I just noticed that another possibility would be to merge this " handling with the other escaping logic in the case branches above, specifically case '\\', e.g.:
...
case '\\', '"':
// Escape the char
outBuffer.append('\\');
outBuffer.append(aChar);
break;
...Though this was just a suggestion anyway, and this PR is already approved, so feel free to ignore.
There was a problem hiding this comment.
I'd be happy to incorporate your suggested changes once you've signed the OCA (I assumed you haven't, as you wrote "I am not an OpenJDK member." Please let me know if that's not the case).
There was a problem hiding this comment.
I have not signed the OCA, however
- Skara forces users commenting on GitHub OpenJDK projects to accept the OpenJDK Terms of Use1, see this example, which I have accepted in the past (otherwise all my comments here would be censored)
- GitHub's Terms of Service require that content submitted to a repository is licensed under the license of the repository
Footnotes
-
I can't find any publicly available information about this Skara feature (maybe there is OpenJDK-internal though), but there are issues related to it in the bug tracker ↩
There was a problem hiding this comment.
I confirmed that I can accept your suggested changes without signing OCA, and I've incorporated them.
|
Thank you for your reviews! |
|
Going to push as commit bca6f51.
Your commit was automatically rebased without conflicts. |
openjdk
Bot
removed
ready
4 participants
Removing obsolete options/code from CLDRConverter, which is now always emits Java code in UTF-8 encoding.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30797/head:pull/30797$ git checkout pull/30797Update a local copy of the PR:
$ git checkout pull/30797$ git pull https://git.openjdk.org/jdk.git pull/30797/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30797View PR using the GUI difftool:
$ git pr show -t 30797Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30797.diff
Using Webrev
Link to Webrev Comment