Skip to content

Define size_t.ByReference and fix macOS sysctl size_t* parameters #1351

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

Merged
merged 2 commits into from
Jun 1, 2021

Conversation

dbwiddis
Copy link
Contributor

@dbwiddis dbwiddis commented May 26, 2021

This fixes the types in SystemB.

I added a ByReference nested class to the existing size_t type already mapped in a superclass, to handle the conditional buffer allocation similarly to NativeLongByReference.

@dbwiddis dbwiddis requested a review from matthiasblaesing May 26, 2021 05:47
@dbwiddis dbwiddis linked an issue May 26, 2021 that may be closed by this pull request
@dbwiddis dbwiddis changed the title Fix macOS sysctl size_t parameters Define size_t.ByReference and fix macOS sysctl size_t* parameters May 29, 2021
@dbwiddis dbwiddis marked this pull request as ready for review May 29, 2021 16:54
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.

Looks like a sane approach. I like the introduction of the size_t.ByReference. Naming it like that makes it less ugly than I feared. I left a few inline comments.

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.

Looks sane. One final idea - I would consider:

  • rename longValue to getLongValue and setValue(long value) to setLongValue (though I'm not sure whether the similarity to Integer#longValuemight be more important
  • directly implement setValue(long) instead of delegating to setValue(size_t) - the same is true for the constructor. The garbadge created by wrapping the long into a size_t is not needed.

@dbwiddis
Copy link
Contributor Author

Looks sane. One final idea - I would consider:

Looks like two ideas. 😁

  • rename longValue to getLongValue and setValue(long value) to setLongValue (though I'm not sure whether the similarity to Integer#longValuemight be more important

I'll think about this but am still leaning toward longValue() as it's used throughout the JNA API and there is not any other getLongValue(). I thought longValue() was closer to the method on IntegerType and as you mention, Java's Number#longValue() is relevant.

This getter directly replaces getValue().longValue() without the object creation overhead...

  • directly implement setValue(long) instead of delegating to setValue(size_t) - the same is true for the constructor. The garbadge created by wrapping the long into a size_t is not needed.

Which is a good point.

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Jun 1, 2021

I've taken some time to think about this:

  1. I still favor longValue(). We don't use getLongValue() anywhere else in JNA and I'm hesitant to change that. The most direct comparison that I can find is in the VARIANT class where getValue().longValue() is just longValue(). (The alternative is adding a getSizeT() to Pointer to match the getNativeLong() method, which I might argue is appropriate.). I am open to toLong() (similar to toString()) which is used in the Windows FILETIME class.

  2. I don't have a problem with changing the getter to bypass the creation of the size_t object, but the setter introduces a small complication. In the case where Native.SIZE_T_SIZE is less than 8, there needs to be a check on the long argument to ensure it is not too large. This is currently done in the constructor for IntegerType (superclass of size_t) in its setValue() method. To respect DRY we could refactor that into a static helper method, or duplicate just the necessary bits (4 vs. 8). Honestly, though, I don't see size_t much differently than NativeLong which is a lot more common and has the same object creation overhead and this feels like a microoptimization.

So ultimately I think I just want to change the getter (whatever name is decided on) to bypass object creation and otherwise leave this PR as is.

@matthiasblaesing
Copy link
Member

Thank you for the consideration. With that arguments, I would keep the changeset asis.

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Jun 1, 2021

I think there is one small optimization that could be done: since the size check is only needed in the <8 case, we could have a conditional where a size of 8 directly sets the value, and a smaller one does the existing check via object creation. That will minimize the overhead to a small number of platforms.

@matthiasblaesing
Copy link
Member

I have no strong feelings here. So would take it any way.

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Jun 1, 2021

Unfortunately that pesky little rule that you can't call another constructor other than as the first call makes the gymnastics to avoid the initialization too unreadable for me to even understand a minute after I wrote it. So I'm just going to merge this as-is.

@dbwiddis dbwiddis merged commit b7b48c9 into java-native-access:master Jun 1, 2021
@dbwiddis dbwiddis deleted the sizet branch June 1, 2021 22:32
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.

macOS sysctl mappings have incorrect type for size arguments
2 participants