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

[Eric McCorkle]

(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/20130816/2e998a0a/eric_mccorkle.vcf