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

[Maurizio Cimadamore]

On Thu, 27 Feb 2020 05:49:09 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.

I like the new patch - my only reservation is about the withAttribute which accepts a Map which, IMHO exposes to much of the implementation (which is why I was suggesting to add, at least in the public API, just a `withAttribute(String, Constable)` which only makes minimal assumptions on the implementation internals. While this is not a blocker, longer term we will need to decide whether to just allow for adding one attribute at  a time or not - if not, then I think a better way to add multiple attributes at a time would be with a builder:

decl.withAttributes(
    a -> a.withAttribute("foo", Foo.class)
              .withAttribute("bar", Bar.class)
);

But I think the need to go down there has to be justified by concrete use cases. Right now we have only one use case, which is internal (so a public method is not even needed). So perhaps, for the time being allowing to add one attribute at a time is a good enough compromise for the public API (and you can push your Map-accepting method down in DeclarationImpl) ?

I'll leave that to your judgement.

P.S. The TreeMaker changes look great - I like how your approach made the code a lot flatter, well done!

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

Marked as reviewed by mcimadamore (Committer).

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