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

Eric McCorkle eric.mccorkle at
Fri Aug 16 15:05:06 PDT 2013

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:

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:
>>> 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:
>>>>>>>> The bug can be found here:
>>>>>>>> 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 : 

More information about the compiler-dev mailing list