Review request for JDK-7118412: Shadowing of type-variables vs. member types

[Eric McCorkle]

Are there any more comments on this one, or is it good to go?

On 08/16/13 18:05, Eric McCorkle wrote:
> Crucible appears to have eaten my review :(
> 
> I've uploaded a new webrev with Alex's changes incorporated, except for
> renaming MethodContext.  It refers to the context (instance vs static)
> in which a call to T.<something, depending on MethodCall> is made.
> 
> The method names in MethodCall are deliberately different.  I use
> "iterator", because the class type variable extends Collection, the
> method names are a way of requiring resolution to have a particular result.
> 
> I also added comments explaining everything.
> 
> The webrev is here:
> http://cr.openjdk.java.net/~emc/7118412/
> 
> On 08/16/13 16:00, Eric McCorkle wrote:
>> (Been gone for 2+ weeks)
>>
>> I will incorporate these changes.
>>
>> However, I'm also going to give the new Crucible review system a try.
>> As such, I've uploaded this patch (not yet with Alex's suggested changes).
>>
>> However, anyone not on the Crucible system should feel free to
>> contribute a review.
>>
>> On 07/24/13 19:51, Alex Buckley wrote:
>>> In the test:
>>>
>>> - Check @summary :-(
>>>
>>> - I wanted to like the nested switch statements in MethodCall.succeeds,
>>> but found them inscrutable. I think more descriptive names for enum
>>> constants would help a lot, e.g. INNER_CLASS. And why are type variables
>>> a.k.a. TYPEVAR so interested in a method called "iterator", can't any
>>> reference type have a method called "iterator" ?
>>>
>>> - Shadowing is not specific to method calls, so I think the succeeds
>>> logic should be moved to an instance method of ShadowingTest. doTest
>>> already has to pass three things to call.succeeds, in order for
>>> MethodCall to work out the answer, so passing 'call' as a fourth
>>> argument is no big deal. It is a good thing to have single-purpose enums
>>> to model the language constructs being combined, since the constructs
>>> are already complicated enough themselves.
>>>
>>> - MethodContext suggests some entity apart from a method that provides
>>> an environment for the method. But in fact, MethodContext represents
>>> whether the method iself is a class method or instance method.
>>> MethodKind would be a better name.
>>>
>>> - InsideDef would be better called MethodDeclaration. (And "NONE"? None
>>> what?) TyVar would be better called TypeParameterDeclaration. (<T
>>> extends ...> is a _type parameter_ declaration, and introduces a _type
>>> variable_ which may be referred to as T in contexts where reference
>>> types are used, e.g. the type in a field declaration. Can you tell I've
>>> been adding annotations on type uses to the JLS recently?)
>>>
>>> - main calls run calls doTest, but doTest is declared before run - and
>>> is private too? Why are some members private and others package-private?
>>>
>>> Alex
>>>
>>> On 7/24/2013 2:42 PM, Eric McCorkle wrote:
>>>> This RFR was dormant for a while pending CCC approval, which happened
>>>> today.  Please note that I've made updates since the last reviews.
>>>>
>>>> I will be running the JCK test suite, as this affects the core compiler.
>>>>
>>>> The updated webrev is here:
>>>> http://cr.openjdk.java.net/~emc/7118412/webrew.02/
>>>>
>>>> On 05/21/13 11:38, Eric McCorkle wrote:
>>>>> I hit a couple of corner cases with the regression tests mid last week.
>>>>>   Expect another update sometime in the near future.
>>>>>
>>>>> On 05/21/13 06:36, Maurizio Cimadamore wrote:
>>>>>> On 21/05/13 11:35, Maurizio Cimadamore wrote:
>>>>>>> Fix looks nice - only bit that seems suspicious is the check for
>>>>>>>
>>>>>>> sym == null
>>>>>>>
>>>>>>> The symbol could be different than null, but still not be a 'valid'
>>>>>>> symbol (i.e. because it's an error). Actually, a lookup should never
>>>>>>> return a null symbol. I suggest you return the 'bestSoFar' (which
>>>>>>> would be typeNotFound') and then use the useful method Symbol.exists()
>>>>>>> which should do what you need (check if the symbol is a valid symbol).
>>>>>>> I think in cases where the found symbol is inaccessible it's important
>>>>>>> to stop and to log the error, you should not keep going recursively
>>>>>>> until you find a correct symbol.
>>>>>> The test also needs some work in order to match the combo test idiom (I
>>>>>> should have sent you an example of such tests already).
>>>>>>
>>>>>> Maurizio
>>>>>>>
>>>>>>> Maurizio
>>>>>>>
>>>>>>> On 15/05/13 16:32, Eric McCorkle wrote:
>>>>>>>> I have updated this patch to correct the shadowing of type
>>>>>>>> variables by
>>>>>>>> both member and static member class definitions.
>>>>>>>>
>>>>>>>> I have also included an improved testing framework that exhaustively
>>>>>>>> tests the possible combinations.
>>>>>>>>
>>>>>>>> Note that a CCC has been filed for this patch, as it changes the way
>>>>>>>> that type names are resolved in the compiler.
>>>>>>>>
>>>>>>>> Please review and comment.
>>>>>>>>
>>>>>>>> On 05/07/13 13:14, Eric McCorkle wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please review this patch which fixes an issue with shadowing by type
>>>>>>>>> variables.
>>>>>>>>>
>>>>>>>>> Note: despite the very small size of this patch, it makes changes to
>>>>>>>>> the
>>>>>>>>> way that types are resolved in scopes, thus it needs to be
>>>>>>>>> treated with
>>>>>>>>> extreme care.  Please review it and the test I have included
>>>>>>>>> carefully.
>>>>>>>>>
>>>>>>>>> The webrev is here:
>>>>>>>>> http://cr.openjdk.java.net/~emc/7118412/webrew.00/
>>>>>>>>>
>>>>>>>>> The bug can be found here:
>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7118412
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Eric
>>>>>>>>>
>>>>>>>
>>>>>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eric_mccorkle.vcf
Type: text/x-vcard
Size: 314 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130820/1b058d4e/eric_mccorkle-0001.vcf