Experience report

[Dan Heidinga]

On Mon, Sep 12, 2022 at 11:36 AM Brian Goetz <brian.goetz at oracle.com> wrote:

> These look good to from an API perspective.  What was the user experience
> like for things like injecting interfaces?  Did the new methods feel clumsy
> or natural?
>

I modified some of the ExampleGallery.java classes to use the new methods
and wrote a similar test case.  The new methods feel pretty natural for
those use cases with the "addingSymbols" method taking the place of
Stream.concat and "deduplicate" replacing Stream.distinct:

           public void accept(ClassBuilder builder, ClassElement element) {
                switch (element) {
                    case Interfaces i:
                        // List<ClassEntry> interfaces =
Stream.concat(i.interfaces().stream(),
                        //
Stream.of(builder.constantPool().classEntry(newIntf)))
                        //                                     .distinct()
                        //                                     .toList();
                        List<ClassEntry> interfaces =
ClassEntry.deduplicate(ClassEntry.addingSymbols(i.interfaces(), newIntf));
                        builder.withInterfaces(interfaces);


>
>
> I'll make a stylistic nit not because I want you to change your code, but
> to remind the world that the language has changed.  The way you write this
> equals() method follows all the Effective Java recommendations:
>
>         public boolean equals(Object o) {
>             if (this == o) return true;
>             if (o instanceof ConcreteMethodHandleEntry m) {
>                 return kind() == m.kind()
>                 && reference.equals(m.reference());
>             }
>             return false;
>         }
>
> but I might write it this way with the language we have now:
>
>         public boolean equals(Object o) {
>             return (this == o)
>             || (o instanceof ConcreteMethodHandleEntry m
>                 && kind() == m.kind()
>                 && reference.equals(m.reference());
>         }
>
> To be clear, the code you wrote is OK, but I've gotten used to writing it
> the second way and I find it more straightforward to read.
>
>
I'll keep the second way in mind for future changes.  It'll probably take a
while for the new pattern to replace the muscle-memory of the old =)

What's the next step for getting these changes merged?

--Dan


>
> +1 from me.
>
> On 9/12/2022 9:16 AM, Dan Heidinga wrote:
>
> I've updated the PR to include the following:
> * CE.adding/addingSymbols methods
> * a List<CE> deduplicate(List<CE>) method that removes duplicate entries
> from the List
> * equals() methods to the ConcreteEntry subclasses
>
> Let me know if this fits with the intended approach for the jdk.classfile
> library.
>
> https://github.com/openjdk/jdk-sandbox/pull/35
> <https://urldefense.com/v3/__https://github.com/openjdk/jdk-sandbox/pull/35__;!!ACWV5N9M2RV99hQ!Kb_eJX0-mhiFRcm4eyz7EFpo2YLLVrvuPNQUH3jcNbjMXCZ5k6pmqHYyNmqZQJKrg3pdDaHAMLWXNeT-_4M$>
>
> --Dan
>
> On Wed, Aug 24, 2022 at 2:55 PM Brian Goetz <brian.goetz at oracle.com>
> wrote:
>
>> I'd say let's give this a try and see what we think of the resulting
>> transformation code.  It feels like right-sizing the overhead at first look.
>>
>> On 8/24/2022 9:22 AM, Dan Heidinga wrote:
>>
>>
>>>
>> 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.
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20220912/76d04dd9/attachment-0001.htm>