> On Oct 21, 2015, at 1:13 PM, Sundararajan Athijegannathan > <sundararajan.athijegannat...@oracle.com> wrote: > > Using enums instead of Strings makes much better reading! Nice!
Thanks! > > * File: NamedOperation.java > > doc comment: gettng -> getting Fixed. > > * File: AbstractJavaLinker.java > > + List<Operation> operations = Arrays.asList( > > final missing? No, it’s reassigned in the loop below. > > +1 > > -Sundar > > On 10/21/2015 4:18 PM, Attila Szegedi wrote: >> Here’s an updated webrev, please review: >> <http://cr.openjdk.java.net/~attila/8139931/webrev.jdk9.01> >> >> I removed CompositeOperation.contains and containsAny operations and in >> Nashorn replaced their use with a new >> NashornCallSiteDescriptor.getFirstStandardOperation method; this allowed us >> to go back to using switch statement everywhere in linker logic where we >> were able to use it earlier, resulting in smaller diff and overall nicer >> code. >> >> Also, following a discussion with Hannes, we now enforce in >> CompositeOperation constructor that no components can themselves be either a >> CompositeOperation or a NamedOperation, and we enforce in NamedOperation >> that its base operation can’t itself be a NamedOperation. >> >> Thanks, >> Attila. >> >>> On Oct 20, 2015, at 12:29 PM, Sundararajan Athijegannathan >>> <sundararajan.athijegannat...@oracle.com> wrote: >>> >>> +1 with changes. >>> >>> On final modifier: nothing specific comes to mind. We can leave it >>> non-final... >>> >>> -Sundar >>> >>> On 10/20/2015 3:09 PM, Attila Szegedi wrote: >>>>> On Oct 20, 2015, at 10:52 AM, Sundararajan Athijegannathan >>>>> <sundararajan.athijegannat...@oracle.com> wrote: >>>>> >>>>> * StandardOperation.java is missing copyright. >>>>> >>>>> * NamedOperation.java is missing copyright. >>>>> >>>>> * CompositeOperation.java is missing copyright. >>>> Indeed; added copyrights. >>>> >>>>> * Can CompositeOperation be final? >>>> Well, it’s hard to see why would someone want to subclass it, but I also >>>> don’t really see why someone shouldn’t be able to do it, to maybe add some >>>> language-specific information and still have it be recognized externally >>>> as CompositeOperation. (Same reasoning goes for NamedOperation, I guess). >>>> Do you have an rationale for making it final? >>>> >>>>> * Unrelated ArrayData change? Unused method? >>>> Yes. I was following a change in the parameter type from String to >>>> Operation and stumbled upon it. >>>> >>>>> * NashornCallSiteDescriptor may have explanation as to why 18 bits are >>>>> sufficient for "program point" [now that flag bits are used for encoding >>>>> operation enums as well] >>>> I agree, I added an explanation. >>>> >>>>> That's all I could find... >>>>> >>>>> +1 >>>>> >>>>> -Sundar >>>>> >>>>> On 10/20/2015 1:41 PM, Attila Szegedi wrote: >>>>>> Please review JDK-8139931 "Introduce Operation objects in Dynalink >>>>>> instead of string encoding" at >>>>>> <http://cr.openjdk.java.net/~attila/8139931/webrev.jdk9> for >>>>>> <https://bugs.openjdk.java.net/browse/JDK-8139931> >>>>>> >>>>>> This is admittedly a big one. It is also the last in the pipeline of the >>>>>> internal Dynalink cleanups, so we’ve reached the end of that! >>>>>> >>>>>> Thanks, >>>>>> Attila. >
