Hi Joe,
I’ll add a comment.
cheers
/Joel
On 10 jun 2014, at 01:09, Joe Darcy <joe.da...@oracle.com> wrote:
> Thanks for the investigation Andrej.
>
> In any case, I'd prefer a comment noting the interned-ness (or mostly
> interned-ness, etc.) of the name to let other readers of the code using ==
> rather than .equals is intentional and not a bug.
>
> Thanks,
>
> -Joe
>
> On 06/09/2014 12:56 PM, Andrej Golovnin wrote:
>> Hi Joel,
>>
>>> IIRC name isn’t actually interned with String.intern() but in the VM:s
>>> Symbol table as a name representing a (Java) method. Should be equivalent,
>>> as long as we don’t start comparing it with == with interned Strings.
>> I think that's not true.
>>
>> The following small test program prints true on the console:
>>
>> public class Test {
>>
>> public static void main(String... argv) throws Exception {
>> Method m = Test.class.getMethod("main", String[].class);
>> System.out.println(m.getName() == new String("main").intern());
>> }
>>
>> }
>>
>> And if you look into reflection.cpp at lines 787-789, you will see following
>> code:
>>
>> Symbol* method_name = method->name();
>> oop name_oop = StringTable::intern(method_name, CHECK_NULL);
>> Handle name = Handle(THREAD, name_oop);
>>
>> And later at the line 798 we have this code:
>>
>> java_lang_reflect_Method::set_name(mh(), name());
>>
>> Therefore I would say the name is actually interned in terms of
>> String.intern().
>> And you can compare the name of a method with == with interned Strings.
>>
>> The same applies to a name of a class and to a name of a field too.
>> Please feel free to correct me.
>>
>> Best regards,
>> Andrej Golovnin
>>
>>
>>> cheers
>>> /Joel
>>>
>>> On 07 Jun 2014, at 22:34, Andrej Golovnin <andrej.golov...@gmail.com> wrote:
>>>
>>>> Hi Joe,
>>>>
>>>>> Sorry for the belated review.
>>>>>
>>>>> Generally the change looks good. One question, in
>>>>>
>>>>> 2803 private boolean matchesNameAndDescriptor(Method m1, Method
>>>>> m2) {
>>>>> 2804 return m1.getReturnType() == m2.getReturnType() &&
>>>>> 2805 m1.getName() == m2.getName() &&
>>>>> 2806 arrayContentsEq(m1.getParameterTypes(),
>>>>> 2807 m2.getParameterTypes());
>>>>> 2808 }
>>>>>
>>>>> Should the equality check on 2805 be .equals rather than == ?
>>>>>
>>>> "==" can be used in this case as the method name is interned by JVM.
>>>> Here is the comment for the field "name" from java.lang.reflect.Method:
>>>>
>>>> // This is guaranteed to be interned by the VM in the 1.4
>>>> // reflection implementation
>>>> private String name;
>>>>
>>>> BTW, in the old version of Class in the line 2766 there was already a
>>>> similar check:
>>>>
>>>> 2764 if (m != null &&
>>>> 2765 m.getReturnType() == toRemove.getReturnType() &&
>>>> 2766 m.getName() == toRemove.getName() &&
>>>> 2767 arrayContentsEq(m.getParameterTypes(),
>>>> 2768 toRemove.getParameterTypes())) {
>>>> 2769 methods[i] = null;
>>>>
>>>>
>>>> Best regards,
>>>> Andrej Golovnin
>>>>
>>>>
>
