<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 12, 2022 at 11:36 AM Brian Goetz <<a href="mailto:brian.goetz@oracle.com" target="_blank">brian.goetz@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<font size="4"><font face="monospace">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?</font></font></div></blockquote><div><br></div><div>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:</div><div><br></div><div> public void accept(ClassBuilder builder, ClassElement element) {<br> switch (element) {<br> case Interfaces i:<br> // List<ClassEntry> interfaces = Stream.concat(i.interfaces().stream(),<br> // Stream.of(builder.constantPool().classEntry(newIntf)))<br> // .distinct()<br> // .toList();<br> List<ClassEntry> interfaces = ClassEntry.deduplicate(ClassEntry.addingSymbols(i.interfaces(), newIntf));<br> builder.withInterfaces(interfaces);<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><font size="4"><font face="monospace"> <br>
<br>
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: <br>
<br>
public boolean equals(Object o) {<br>
if (this == o) return true;<br>
if (o instanceof ConcreteMethodHandleEntry m) {<br>
return kind() == m.kind()<br>
&& reference.equals(m.reference());<br>
}<br>
return false;<br>
}<br>
<br>
but I might write it this way with the language we have now:<br>
<br>
public boolean equals(Object o) {<br>
return (this == o) <br>
|| (o instanceof ConcreteMethodHandleEntry m<br>
&& kind() == m.kind()<br>
&& reference.equals(m.reference());<br>
}<br>
<br>
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. <br>
<br></font></font></div></blockquote><div><br></div><div>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 =)</div><div><br></div><div>What's the next step for getting these changes merged?</div><div><br></div><div>--Dan</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><font size="4"><font face="monospace">
<br>
+1 from me. <br>
</font></font><br>
<div>On 9/12/2022 9:16 AM, Dan Heidinga
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">I've updated the PR to include the following:
<div>* CE.adding/addingSymbols methods</div>
<div>* a List<CE> deduplicate(List<CE>) method that
removes duplicate entries from the List</div>
<div>* equals() methods to the ConcreteEntry subclasses</div>
<div><br>
</div>
<div>Let me know if this fits with the intended approach for the
jdk.classfile library.</div>
<div><br>
</div>
<div><a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk-sandbox/pull/35__;!!ACWV5N9M2RV99hQ!Kb_eJX0-mhiFRcm4eyz7EFpo2YLLVrvuPNQUH3jcNbjMXCZ5k6pmqHYyNmqZQJKrg3pdDaHAMLWXNeT-_4M$" target="_blank">https://github.com/openjdk/jdk-sandbox/pull/35</a><br>
</div>
<div><br>
</div>
<div>--Dan</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Aug 24, 2022 at 2:55
PM Brian Goetz <<a href="mailto:brian.goetz@oracle.com" target="_blank">brian.goetz@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div> <font size="4"><font face="monospace">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.</font></font><br>
<br>
<div>On 8/24/2022 9:22 AM, Dan Heidinga wrote:<br>
</div>
<blockquote type="cite">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
<div><br>
</div>
<div>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.</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</div>
</blockquote></div></div>