RFR: 8152404: Stabilize PackageEntry::package_exports_do

[harold seigel]

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
>>>
>