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

Integrate reported quality issues #1570

Merged

Conversation

matthiasblaesing
Copy link
Member

Closes: #1560
Closes: #1561

@matthiasblaesing
Copy link
Member Author

@dbwiddis could you please have a look at this? There are differences regarding the suggested approach by @sebbASF, but the core matches. Where I took a different route:

  • NativeLibraryDisposer.handle does not need to be volatile. All access to that variable is guarded by the synchronized keyword. This establishes a happens-before relationship for all other synchronized blocks on the same monitor.
  • NativeLibrary#callFlags and NativeLibrary#options are now private. I did not find a use-site for callFlags outside NativeLibrary and NativeLibrary#options has a public accessor getOptions, which should be used

@sebbASF
Copy link

sebbASF commented Dec 5, 2023

Good point; not sure how I missed that. I no longer think that handle needs to be volatile.

Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Looks sane to me. One suggestion (see also #1571)

@@ -245,7 +245,7 @@ public static Function getFunction(Pointer p, int callFlags, String encoding) {
this.library = library;
this.functionName = functionName;
this.callFlags = callFlags;
this.options = library.options;
this.options = library.getOptions();
Copy link
Contributor

Choose a reason for hiding this comment

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

This map is still mutable; if someone modified the library map (e.g. getOptions.clear() that would echo here.

Now that we are on JDK8 I'd suggest making this version of it an unmodifiable copy. (I'd suggest making the getter return unmodifiable but that's probably a breaking change.)

Suggested change
this.options = library.getOptions();
this.options = Map.copyOf(library.getOptions());

@matthiasblaesing
Copy link
Member Author

Thanks for the review. I'll integrate as if for now, I think the discussion about potentially user facing changes needs some more time.

@matthiasblaesing matthiasblaesing merged commit 6706361 into java-native-access:master Dec 5, 2023
8 checks passed
@matthiasblaesing matthiasblaesing deleted the cleanup branch July 13, 2024 19:39
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.

3 participants