<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>