RFR: 8152404: Stabilize PackageEntry::package_exports_do

[Rachel Protacio]

Thanks for the review, Harold! I'll check it in.

Rachel


On 6/3/2016 10:44 AM, harold seigel wrote:
> Hi Rachel,
>
> The changes look good!
>
> Thanks, Harold
>
>
> On 6/3/2016 10:29 AM, Rachel Protacio wrote:
>> Thanks for the review, David. I've fixed that comment discrepancy.
>>
>> Rachel
>>
>>
>> On 6/3/2016 12:40 AM, David Holmes wrote:
>>> Hi Rachel,
>>>
>>> On 3/06/2016 1:36 AM, Rachel Protacio wrote:
>>>> Hi, David,
>>>>
>>>> New webrev: http://cr.openjdk.java.net/~rprotacio/8152404.01/
>>>>
>>>> Replies inline.
>>>>
>>>> On 5/25/2016 5:10 PM, David Holmes wrote:
>>>>> Hi Rachel,
>>>>>
>>>>> On 26/05/2016 1:10 AM, Rachel Protacio wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review this bug fix for some module export checking code. The
>>>>>> PackageEntry::package_exports_do() function had assumed calling
>>>>>> is_qual_exported() would verify that _qualified_exports was not 
>>>>>> NULL,
>>>>>> but as that was not the case, I've re-defined the PackageEntry 
>>>>>> member
>>>>>> variables and export checking functions.
>>>>>
>>>>> In src/share/vm/classfile/packageEntry.hpp the documentation no 
>>>>> longer
>>>>> makes it clear how the three variables are defined for the four
>>>>> different situation ie for an unnamed export what are the values of
>>>>> _qualified_exports and _is_exported_unqualified?
>>>> I've updated the comment at the top of the file (thanks, Lois!). Does
>>>> this make it more clear?
>>>
>>> Yes - thanks.
>>>
>>> This comment is no longer accurate though:
>>>
>>>  37 //   - a flag indicating if package is exported, either 
>>> qualifiedly or
>>>   38 //     unqualifiedly.
>>>
>>> as the flag is now only for the unqualified case.
>>>
>>>>>
>>>>> Overall (given I'm not familiar with the different export 
>>>>> qualities) I
>>>>> found it a little unclear how the different states were defined and
>>>>> what the legal transitions are between them. I would have expected to
>>>>> see more assertions regarding those three state variables, and checks
>>>>> for when they get set or cleared. In one place you actually removed
>>>>> such an assertion.
>>>> Good point. I had removed it because I removed the _is_exported
>>>> variable, but I've now put assertions into the .hpp state functions.
>>>
>>> Looks good.
>>>
>>> No further comments from me.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Thanks,
>>>> Rachel
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Passes JPRT, jck (vm, lang, and api/java_lang), and RBT 
>>>>>> hotspot_all and
>>>>>> non-colo tests.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8152404
>>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8152404.00/
>>>>>>
>>>>>> Thanks,
>>>>>> Rachel
>>>>
>>
>