[foreign-jextract] [Rev 01] RFR: 8239809: Need API to access attributes

[Maurizio Cimadamore]

On Tue, 25 Feb 2020 20:50:05 GMT, Henry Jen <henryjen at openjdk.org> wrote:

>> Add API to allow associate attributes with Declaration.
>> 
>> An attribute can have multiple values, thus a list of ConstantDesc.
>> 
>> The test case is based on [JDK-8223105](https://bugs.openjdk.java.net/browse/JDK-8223105), Windows test is simply parsing at this point, need to add validation as the header is implemented differently. 
>> 
>> We may want to add a more generic test case with some standard attributes.
> 
> The pull request has been updated with 1 additional commit.

As stated before, I think it would be useful for the attribute code to be more structured. If you don't want to add public APIs for now, fine, but we should at least add an internal method like:

abstract DeclarationImpl::withAttributes(Map<String, ConstantDesc>

Which is then implemented in all the subclasses (the current approach makes it easy to miss cases, as the ScopedImpl case shows). Once we have that, I believe the majority of the changes to TreeMaker can be reverted too, and that attributes can simply be attached before the parsed declaration is returned.

src/jdk.incubator.jextract/share/classes/jdk/incubator/jextract/Declaration.java line 69:

> 68:      */
> 69:     Set<String> attributeNames();
> 70: 

I find it odd that the API has ways to query attributes, but no way to set them. I really think that withAttribute/dropAttributes should be a pretty minimal way to get there.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/DeclarationImpl.java line 79:

> 78: 
> 79:         private VariableImpl(Type type, Optional<MemoryLayout> layout, Variable.Kind kind, String name, Position pos, Map<String, List<ConstantDesc>> attrs) {
> 80:             super(name, pos, attrs);

Rather than changing these factories to add attribute variants, consider either using `withAttribute` and drop the changes, or add an internal method in DeclarationImp which clones a declaration and adds annotations (even using a single Map).

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TreeMaker.java line 94:

> 93:         Objects.requireNonNull(c);
> 94:         Map<String, List<ConstantDesc>> attrs = collectAttributes(c);
> 95:         switch (c.kind()) {

Rather than collecting attributes upfront, and then having to pass down the attributes to all the various createXYZ functions, wouldn't it be easier to take the parsed declaration and adding the attributes after the fact? That way the createXYZ could remain unchanged and the attribute changes would be more orthogonal. This could be implemented on top of either a public or an impl-only withAttribute functionality.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/DeclarationImpl.java line 168:

> 167: 
> 168:         ScopedImpl(Kind kind, Optional<MemoryLayout> optLayout, List<Declaration> declarations, String name, Position pos) {
> 169:             super(name, pos, Collections.emptyMap());

No addition of an annotation-accepting ScopeImpl constructor?

-------------



PR: https://git.openjdk.java.net/panama-foreign/pull/33