Experience report

[Dan Heidinga]

On Tue, Aug 16, 2022 at 10:28 AM Brian Goetz <brian.goetz at oracle.com> wrote:

>
>
> On 8/15/2022 4:26 PM, Dan Heidinga wrote:
> > Thanks for the feedback, Brian.
> >
> > I've worked through the suggestions on the various ways to update an
> > Attribute {use a stateful ClassTransform, use
> > builder.original().get()} and found the "original().get()" approach
> > felt the cleanest and clearest for what I was doing.
>
> By that, do you mean "cleanest of the terrible options", or "seemed a
> good enough option"?
>

After playing more with both options, I think both have their uses.

original().get() works well for quick prototyping and is easy to get
something up and running.  In my case, it's guaranteed that the "original"
ClassModel exists so I don't have to add any extra error handling for that
case.

ofStateful() seems like a more generally applicable solution to detecting
something, holding it, and processing it later when you aren't guaranteed
an original ClassModel exists (not entirely sure when that would be the
case).  This is really useful for the set of Attributes that have the
"there can only be one" invariant.  What made it feel like too much was
trying to write the "new ClassTransform() { ...} " inline.  Factoring that
out into a separately named class makes the whole approach much clearer.

My current approach is to use a separate class for
SingleAttributeTransforms that accepts three arguments:
* the Attribute class to detect
* an ifPresent Function argument, and
* an ifAbsent Supplier argument.

I prototyped something [0] that works for my use case (though the generics
aren't right yet as not all Attributes are ClassElements) but haven't
thought about how well it would generalize to allow tracking attributes
that are one-per method.  Current use of it look like:

   ClassTransform singleAttribT(final ArrayList<ClassDesc> nestMembers) {
         return ClassTransform.ofStateful( () -> {
             return new SingleAttributeTransform<NestMembersAttribute>(
                 NestMembersAttribute.class,
                 nma -> { return
NestMembersAttribute.withSymbols((NestMembersAttribute)nma, nestMembers);},
                 () -> { return
NestMembersAttribute.ofSymbols(nestMembers);});
             }
         );
     }

All that to say - original().get() works for quickly hacking something up
and ofStateful is likely the solution to reach for when writing something
more maintainable.  OfStateful benefits from defining a separate class
out-of-line and then resulting code looks pretty good.

[0]
https://github.com/DanHeidinga/jdk-sandbox/pull/1/commits/cfa5acf8594075b8786aa1bf86dd8c8850175728

>
> > While working on that, I also added the ::with & ::withSymbols methods
> > I had proposed and opened a PR [0] to see if the fleshed out API fit
> > with the general way you wanted to evolve this.
>
> I had a look at the API suggestions and I have a counter-suggestion.
>
> The operation that you are trying to enable is adding to (and I predict
> eventually, removing from) list-holding attributes.  You propose four
> new methods for each attribute:
>
>      NMA with(NMA base, List<CE> additions)
>      NMA with(NMA base, CE... additions)
>      NMA withSymbols(NMA base, List<CD> additions)
>      NMA withSymbols(NMA base, CD... additions)
>
> So that's 2 representations (CE/CD) x 2 API styles (list, varargs) x N
> attributes (4N), which will get worse if the attribute is not merely a
> holder for a list, but might have other stuff to be modified too.  If
> you want to support removals, that's 2x more.
>
> Really what you need is four methods, somewhere:
>
>      List<CE> adding(List<CE>, List<CE>)
>      List<CE> adding(List<CE>, CE...)
>      List<CE> addingSymbols(List<CE>, List<CD>)
>      List<CE> addingSymbols(List<CE>, CD...)
>
> and invoke it like:
>
>      case NMA a -> NMA.of(adding(a.nestMembers(), NEW_STUFF));
>
> And this would cover all attributes, plus would support dealing with
> attributes that have more than just a List<CE> in them.  The question is
> where to put these to make them discoverable.
>

I took another pass through the Attributes that deal with Lists of things
and most cases are well handled by existing List helpers.  It's only the
CE/CD cases that need data conversion and would therefore benefit from
additional methods to create their lists.

If we expect to be content with these 4 methods, then adding them as static
methods on ClassEntry seems reasonable. It makes them easy to find and
avoids a garbage "Helpers/Utils" class.  If we expect more of these methods
(and I currently don't but that may be lack of imagination), then a garbage
class might be better.

--Dan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20220824/759993a5/attachment-0001.htm>