Skip to content

define the structure's field order with metadata #954

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

define the structure's field order with metadata #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Features
* Bind `com.sun.jna.platform.win32.Kernel32.ExpandEnvironmentStrings` and add helper method for it as `Kernel32Util#expandEnvironmentStrings` - [@matthiasblaesing](https://github.com/matthiasblaesing).
* [#935](https://github.com/java-native-access/jna/pull/935): Add RegConnectRegistry to Advapi32 mappings. - [@cxcorp](https://github.com/cxcorp).
* [#947](https://github.com/java-native-access/jna/pull/947): Allow retrieval of `ACEs` from `com.sun.jna.platform.win32.WinNT.ACL` even if the contained `ACE` is not currently supported - [@jrobhoward](https://github.com/jrobhoward).
* Add `c.s.j.Structure.FieldOrder` annotation to define the field order of a structures without implementing `Structure#getFieldOrder()` - [@idosu](https://github.com/idosu).

Bug Fixes
---------
Expand Down
98 changes: 80 additions & 18 deletions src/com/sun/jna/Structure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@
*/
package com.sun.jna;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Array;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
Expand All @@ -38,6 +43,7 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -66,11 +72,12 @@
* public. If your structure is to have no fields of its own, it must be
* declared abstract.
* </p>
* <p>You <em>must</em> define {@link #getFieldOrder} to return a List of
* field names (Strings) indicating the proper order of the fields. When
* dealing with multiple levels of subclasses of Structure, you must add to
* the list provided by the superclass {@link #getFieldOrder}
* the fields defined in the current class.
* <p>You <em>must</em> annotate the class with {@link FieldOrder} or implement
* {@link #getFieldOrder}, whichever you choose it must contain the field names
* (Strings) indicating the proper order of the fields. If you chose to implement
* {@link #getFieldOrder} notice that when dealing with multiple levels of
* subclasses of Structure, you must add to the list provided by the superclass
* {@link #getFieldOrder} the fields defined in the current class.
* </p>
* <p>In the past, most VMs would return them in a predictable order, but the JVM
* spec does not require it, so {@link #getFieldOrder} is now required to
Expand Down Expand Up @@ -865,20 +872,66 @@ protected void writeField(StructField structField) {
}
}

/** Return this Structure's field names in their proper order. For
* example,
/** Used to declare fields order as metadata instead of method.
* example:
* <pre><code>
* protected List getFieldOrder() {
* return Arrays.asList(new String[] { ... });
* // New
* {@literal @}FieldOrder({ "n", "s" })
* class Parent extends Structure {
* public int n;
* public String s;
* }
* {@literal @}FieldOrder({ "d", "c" })
* class Son extends Parent {
* public double d;
* public char c;
* }
* // Old
* class Parent extends Structure {
* public int n;
* public String s;
* protected List<String> getFieldOrder() {
* return Arrays.asList("n", "s");
* }
* }
* class Son extends Parent {
* public double d;
* public char c;
* protected List<String> getFieldOrder() {
* List<String> fields = new LinkedList<String>(super.getFieldOrder());
* fields.addAll(Arrays.asList("d", "c"));
* return fields;
* }
* }
* </code></pre>
*/
@Documented
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface FieldOrder {
String[] value();
}

/** Returns this Structure's field names in their proper order.<br>
*
* When defining a new {@link Structure} you shouldn't override this
* method, but use {@link FieldOrder} annotation to define your field
* order(this also works with inheritance)<br>
*
* If you want to do something non-standard you can override the method
* and define it as followed
* <pre><code>
* protected List<String> getFieldOrder() {
* return Arrays.asList(...);
* }
* </code></pre>
* <strong>IMPORTANT</strong>
* When deriving from an existing Structure subclass, ensure that
* you augment the list provided by the superclass, e.g.
* <pre><code>
* protected List getFieldOrder() {
* List fields = new ArrayList(super.getFieldOrder());
* fields.addAll(Arrays.asList(new String[] { ... }));
* protected List<String> getFieldOrder() {
* List<String> fields = new LinkedList<String>(super.getFieldOrder());
* fields.addAll(Arrays.asList(...));
* return fields;
* }
* </code></pre>
Expand All @@ -888,7 +941,19 @@ protected void writeField(StructField structField) {
* guaranteed to be predictable.
* @return ordered list of field names
*/
protected abstract List<String> getFieldOrder();
// TODO(idosu 28 Apr 2018): Maybe deprecate this method to let users know they should use @FieldOrder
protected List<String> getFieldOrder() {
List<String> fields = new LinkedList<String>();
for (Class<?> clazz = getClass(); clazz != Structure.class; clazz = clazz.getSuperclass()) {
Copy link
Member

@matthiasblaesing matthiasblaesing Apr 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If you walk the inheritance structure here, you'll miss classes, that extend Structure, but don't use the annotation. So what I would do at this point:

  • fetch the field order for the super class via a super.getFieldOrder() call
  • extract the local order from the annotation and append it

This way you can mix the definitions. As the order is caches, this should not have a big impact on performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not fixed

I thought about it but I don't agree, because if a class overrides this method no sub class can call the default implementation i.e. getting with annotation

class Parent extends Structure {
    @Override
    protected List<String> getFieldOrder() {
        return Arrays.asList("a");
    }
}

class Son extends C {
    @Override
    protected List<String> getFieldOrder() {
        // super.getFieldOrder() returns [ "a" ]
        // Structure.super.getFieldOrder() - this is not valid
    }
}

plus calling getClass() in super.getFieldOrder() will result in the same class as it would if you write it here and therefore will result in a StackOverflowException.

Could you show me an example perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW I had another idea to check in this.fieldOrder if we already got fields for the super class and use them instead of iterating the inheritance structure. what do you think, does it worth the effort?

Copy link
Member

Choose a reason for hiding this comment

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

You are right - subclasses can still call super#getFieldOrder and so build a complete list if a super class uses the @FieldOrder annotation. It will fail if a child class is using the annotation and the super class is not. In this case the member check at runtime will blow. For the 5.0.0 release it would be ok to update all structures in the platform.

Copy link
Contributor Author

@idosu idosu May 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Notice that if the the parent class's getFieldOrder method was overridden there is no way to call the original code of Structure#getFieldOrder therefor if the user wants to extend the parent class he must override getFieldOrder and add the new fields manually.

I can create an helper method to use when using an old library(without the usage of @FieldOrder) so the user could just override getFieldOrder and return getNonAnnotatedAndAnnotatedFieldOrder(), but it seems to be error prone(in this configuration: classWithAnnotation extends classWithout extends classWith extends classWithout)

protected List<String> getFieldOrder() {
    return getAnnotatedFieldOrder();
}

protected final List<String> getAnnotatedFieldOrder() {
    // the code of the current implementation of getFieldOrder
}

protected final List<String> getNonAnnotatedAndAnnotatedFieldOrder() {
    List<String> parent = getFieldOrder();
    List<String> annotated = getAnnotatedFieldOrder();

    List<String> fieldOrder = new ArrayList<String>(parent.size() + annotated.size());
    fieldOrder.addAll(parent);
    fieldOrder.addAll(annotated);

    return fieldOrder;
}

Should I add it anyway?

FieldOrder order = clazz.getAnnotation(FieldOrder.class);
if (order != null) {
fields.addAll(0, Arrays.asList(order.value()));
}
}

// fields.isEmpty() can be true because it is check somewhere else
return Collections.unmodifiableList(fields);
}

/** Sort the structure fields according to the given array of names.
* @param fields list of fields to be sorted
Expand Down Expand Up @@ -1855,6 +1920,7 @@ public String toString() {
* structure for use by libffi. The lifecycle of this structure is easier
* to manage on the Java side than in native code.
*/
@FieldOrder({ "size", "alignment", "type", "elements" })
static class FFIType extends Structure {
public static class size_t extends IntegerType {
private static final long serialVersionUID = 1L;
Expand Down Expand Up @@ -1951,11 +2017,7 @@ private FFIType(Object array, Class<?> type) {
}
init(els);
}

@Override
protected List<String> getFieldOrder() {
return Arrays.asList(new String[] { "size", "alignment", "type", "elements" });
}

private void init(Pointer[] els) {
elements = new Memory(Native.POINTER_SIZE * els.length);
elements.write(0, els, 0, els.length);
Expand Down
Loading