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

Use JDK7 and JDK8 Code Features #1564

Merged
merged 5 commits into from
Nov 20, 2023
Merged

Conversation

dbwiddis
Copy link
Contributor

@dbwiddis dbwiddis commented Nov 20, 2023

Used IDE cleanup tool with manual reverting of the anonymous inner class fixes to apply the following (each in a separate commit for ease of review or if you only want a portion of the changes):

  • Diamond Operators
  • Try-with-resources
  • Try-multi-catch
  • Constants in preference to system properties

@dbwiddis dbwiddis force-pushed the diamond branch 3 times, most recently from 59471c8 to a464ceb Compare November 20, 2023 09:31
Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You found a whole lot of diamonds ;-)

Thanks for preparing this. I left two inline comments, that might make sense.

@@ -202,8 +204,8 @@ public void testGetOptionsForDirectMappingWithMemberInitializer() {
};
final TypeMapper mapper = new DefaultTypeMapper();
final int alignment = Structure.ALIGN_NONE;
final String encoding = System.getProperty("file.encoding");
Map<String, Object> options = new HashMap<String, Object>();
final String encoding = Charset.defaultCharset().displayName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure here, but the display name, that is even localized, seems wrong. The canonical name of the charset seems to be a better fit:

Suggested change
final String encoding = Charset.defaultCharset().displayName();
final String encoding = Charset.defaultCharset().name();

Same would apply in line 228

@@ -85,7 +85,7 @@ public void testLongStringGeneration() {
}

public void testCustomStringEncoding() throws Exception {
final String ENCODING = System.getProperty("file.encoding");
final String ENCODING = Charset.defaultCharset().displayName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in DirectTest (same on line 480)

@dbwiddis
Copy link
Contributor Author

You found a whole lot of diamonds ;-)

To be fair, Eclipse found them, plus a few others that it shouldn't have. The "Clean Up" tool made this easy.

I left two inline comments, that might make sense.

The suggestion was Eclipse's. Given Eclipse's failure with the diamonds on anonymous subclasses, I trust it less now, so looking into this.

  • The file.encoding system property is accessible via the constant jdk.internal.util.StaticProperty.fileEncoding() but that's internal.
  • That method is called in Charset.defaultCharset() as the argument to charsetForName()
  • There the argument is canonicalized when looking up the actual charset (so it could be an alias) so the actual file encoding String is lost. But the Charset name field is populated with the canonical name
  • Both name() and displayName() with no arguments return the name field without considering locale, so your suggestion makes no difference in the default case, but...
  • Both of those methods can be overridden by subclasses as well, and the javadoc for displayName() indicates it's intended to be "human readable" so is probably not the best. name() returns the canonical name.

So it looks like your suggestion is a good one, although in theory if someone had a "human readable" file.encoding it might be closer to the truth. Perhaps that's what Eclipse's devs were thinking.

I'll update to your suggestion. It's in a test class anyway so not critical, worst case is a failed test someone has to debug.

@dbwiddis dbwiddis merged commit 9e5243f into java-native-access:master Nov 20, 2023
8 checks passed
@dbwiddis
Copy link
Contributor Author

Will follow up with a similar PR for platform tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants