Review request for JDK-8139931: Introduce Operation objects in Dynalink instead of string encoding

[Attila Szegedi]

> On Oct 21, 2015, at 1:13 PM, Sundararajan Athijegannathan <sundararajan.athijegannathan at 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.athijegannathan at 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.athijegannathan at 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.
>