Hi Svetlana, Looks okay. I approve pushing the fix with the following change to the test: have the expected output align with the given output. The webrev shows 63 "FirstInnerClassGeneric<Dummy>$SecondInnerClassGeneric<Dummy>") 64 public FirstInnerClassGeneric<Dummy>.SecondInnerClassGeneric<Dummy> foo1() and I think this is clearer as 63 "FirstInnerClassGeneric<Dummy>$SecondInnerClassGeneric<Dummy>") 64 public FirstInnerClassGeneric<Dummy>.SecondInnerClassGeneric<Dummy> foo1() even if the spacing is less conventional. No need to generate another webrev. Cheers, -Joe On 7/13/2016 8:35 AM, Svetlana Nikandrova wrote:
Kindly reminder.
On 07.07.2016 18:49, Svetlana Nikandrova wrote:
Hello all,
seems like Joe is busy right now, so meanwhile I'll be happy to hear other opinions on this topic.
Thank you, Svetlana
On 30.06.2016 19:44, Svetlana Nikandrova wrote:
Hello Joe,
thank you for your advice! GenericStringTest looks really good with annotations. I refactored my test, please see updated webrev: http://cr.openjdk.java.net/~snikandrova/8054213/webrev.01/ <http://cr.openjdk.java.net/%7Esnikandrova/8054213/webrev.01/>
As for "." vs "$" let me provide an example: E.g. we have class public class Test { public Nested1<Test>.Nested2<Test> foo() { return null; } public class Nested1<T> { public class Nested2<T>{} } } The output of the System.out.println(Test.class.getMethod("foo", null).getReturnType()); Output: class Test$Nested1$Nested2 (nested classes divided by "$") while System.out.println(Test.class.getMethod("foo", null).getGenericReturnType()); Output: Test$Nested1<Test>.Nested2<Test> (nested classes divided by "." if enclosed by parametrized type and "$" in other cases). I think it's a little bit strange to have different separators for inner classes depended on is it nested by parametrized or raw type.
Thank you, Svetlana
On 28.06.2016 22:02, joe darcy wrote:
Hello Svetlana,
I'm not convinced '$' should be replaced with '.' in this context as '.' is what the separator used in source code.
In any case, the test should be restructured. I recommend declaring an annotation type to hold the expected to string output. You can them loop over the methods from the class object and for the methods with the annotation verifying the toString output is as expected. For a guide see
test/java/lang/reflect/Constructor/GenericStringTest.java
Thanks,
-Joe
On 6/28/2016 11:25 AM, Svetlana Nikandrova wrote:
May be someone can find a minute?
On 27.06.2016 21:25, Svetlana Nikandrova wrote:
Kindly reminder
On 24.06.2016 14:58, Svetlana Nikandrova wrote: > Hello, > > let me try again with updated version of webrev: > Webrev: > http://cr.openjdk.java.net/~snikandrova/8054213/webrev.01/ > <http://cr.openjdk.java.net/%7Esnikandrova/8054213/webrev.01/> > Issue: > https://bugs.openjdk.java.net/browse/JDK-8054213 > > Many thanks goes to Sergey Ustimenko for his valuable advises. I > believe fix looks nicer now. > JPRT tested. > > Thank you, > Svetlana > > On 17.06.2016 21:45, Svetlana Nikandrova wrote: >> Hello, >> >> could you please review this fix for toString() method of >> ParameterizedTypeImpl. >> The problem is that when we obtain simple name of nested type >> shared prefix is only removed for ParameterizedType owner. We >> need to remove it for other cases too. >> >> Please note that I also changed delimiter between outer and >> inner class from "." to "$". I believe as "$" is usually used >> to separate nested class name "." looks inconsistent and >> confusing. >> Let me know if you have any objections. >> >> Webrev: >> http://cr.openjdk.java.net/~msolovie/8054213/webrev.00/ >> <http://cr.openjdk.java.net/%7Emsolovie/8054213/webrev.00/> >> Issue: >> https://bugs.openjdk.java.net/browse/JDK-8054213 >> >> Thank you, >> Svetlana >