Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Document fact that Structure.setFieldOrder() should always be invoked #84

Closed
cowwoc opened this Issue · 10 comments

2 participants

@cowwoc

Currently Structure.setFieldOrder() indicates that it only needs to be invoked on platforms where "field order as returned by Class.getFields() is not predictable." But in fact, field order is not guaranteed to be predictable on any platform.

The specification for Class.getFields() explicitly states "The elements in the array returned are not sorted and are not in any particular order." It's a mistake to imply that users may skip this required step. Look at what happened in Java7. Class.getDeclaredMethods() used to return methods in a predictable order but it no longer does: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7023180

We should require users to invoke this method from now on, otherwise their applications are likely to break in the near future.

@twall
Owner

I agree. Initially field order was left out because all desktop VMs empirically had predictable field order, but now given the range of VMs available it's better to stick to the spec.

@twall
Owner
@twall
Owner

Currently working on this one. Still have a few outstanding testsuite failures due to the (now) required field order initialization.

@twall
Owner

I propose making setFieldOrder() final, and getFieldOrder() abstract. This will flag 95% of code that will need modification in order to properly declare field order. Structures subclassed more than once would need special attention (since the superclass having defined the abstract method would hide the absence of additional field order for the subclass).

This would warrant a version bump to 3.5, for the introduced incompatibility.

@twall
Owner

See corresponding branch, explicit-structure-field-order.

@twall twall referenced this issue from a commit
@twall address issue #84 a947dfd
@cowwoc

On a related note, I just ran across this exception (on the Android branch):

 at com.sun.jna.Structure.getFields(Structure.java:794)
 at com.sun.jna.Structure.deriveLayout(Structure.java:875)
 at com.sun.jna.Structure.calculateSize(Structure.java:834)
 at com.sun.jna.Structure.allocateMemory(Structure.java:322)
 at com.sun.jna.Structure.ensureAllocated(Structure.java:307)
 at com.sun.jna.Structure.ensureAllocated(Structure.java:297)
 at com.sun.jna.Structure.size(Structure.java:351)
 at com.sun.jna.Native.getNativeSize(Native.java:976)
 at com.sun.jna.Structure.getNativeSize(Structure.java:1631)
 at com.sun.jna.Structure.deriveLayout(Structure.java:961)
 at com.sun.jna.Structure.calculateSize(Structure.java:834)
 at com.sun.jna.Structure.calculateSize(Structure.java:821)
 at com.sun.jna.Structure.allocateMemory(Structure.java:334)
 at com.sun.jna.Structure.<init>(Structure.java:195)
 at com.sun.jna.Structure.<init>(Structure.java:185)
 at com.sun.jna.Structure.<init>(Structure.java:181)
 at com.sun.jna.Structure.<init>(Structure.java:172)
 [snip]

The problem is this:

  1. When Structure's default constructor is invoked, it attempt to auto-allocated the memory buffer.
  2. However, if field ordering is not predictable (and now it never is) then getFields() throws an Error. The caller methods expect IllegalStateException() if the size cannot be calculated so they fail to handle it correctly.
  3. I've changed the Error into a IllegalStateException. As well, I removed memory auto-allocation in Structure's default constructor (since you cannot determine memory size without field ordering because of memory alignment will produce different sizes depending on the ordering).
@cowwoc

I reviewed your commit and it looks good. Is there any reason you didn't merge these changes into the master and android branches and close this issue?

PS: Do you get notifications when I post comments on your commits as I did here? a947dfd

@twall
Owner
@twall
Owner
@twall
Owner

Applied to master in 2ff4d20.

@twall twall closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.