RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Calvin Cheung calvin.cheung at oracle.com
Fri Jan 5 00:56:24 UTC 2018


Thanks again - Ioi and Serguei.

Calvin

On 1/4/18, 4:41 PM, Ioi Lam wrote:
> Looks good!
>
> Thanks
>
> - Ioi
>
>
> On 1/4/18 2:12 PM, Calvin Cheung wrote:
>> Hi Serguei,
>>
>> Thanks for your review.
>>
>> Here's an updated webrev with your suggested simplifications:
>>
>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.03/
>>
>> thanks,
>> Calvin
>>
>> On 1/4/18, 12:48 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Calvin,
>>>
>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/src/hotspot/os/windows/os_windows.cpp.frames.html 
>>>
>>>
>>> I'd suggest a minor simplification.
>>> 4397   bool is_empty = false;
>>> Set the is_empty to 'true' instead.
>>> Then you can remove this alternative:
>>> 4427   if (f == INVALID_HANDLE_VALUE) {
>>> 4428     is_empty = true;
>>> It would look like this:
>>>     if (f != INVALID_HANDLE_VALUE) {
>>>       while (is_empty&&  ::FindNextFileW(f,&fd)) {
>>>       . . .
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 1/4/18 10:33, Calvin Cheung wrote:
>>>> Hi Ioi,
>>>>
>>>> Thanks for your review.
>>>>
>>>> I've made your suggested changes in the test case and ran it on 10 
>>>> different windows hosts.
>>>>
>>>> updated webrev:
>>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>> On 1/3/18, 6:02 PM, Ioi Lam wrote:
>>>>> Hi Calvin,
>>>>>
>>>>> The code changes look good. I would suggesting running the tests 
>>>>> in our test harness several times so you can run on different 
>>>>> windows hosts.
>>>>>
>>>>>
>>>>> For the test case, I think there are too many use of 
>>>>> "DoesntMatter" which may be confusing. I would suggest adding
>>>>>
>>>>>     String classList[] = {"java/lang/Object"};
>>>>>
>>>>> and replacingall 'TestCommon.list("DoesntMatter")' with classList.
>>>>>
>>>>> Also,
>>>>>
>>>>>    File subDir = new File(longDir, "DoesntMatter");
>>>>> ->
>>>>>    File subDir = new File(longDir, "subdir");
>>>>>
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>>>
>>>>> On 12/20/17 2:00 PM, Calvin Cheung wrote:
>>>>>> I've updated the change slightly:
>>>>>> - replaced the do-while loop with a while loop;
>>>>>> - free(search_path) should be os::free(search_path)
>>>>>>
>>>>>> updated webrev: 
>>>>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.01/
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>>
>>>>>> On 12/19/17, 5:01 PM, Calvin Cheung wrote:
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8192927
>>>>>>>
>>>>>>> This change is targeted for JDK 11.
>>>>>>> Please refer to the comment in the bug for a description of the 
>>>>>>> change. It also handles path longer than MAX_PATH.
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/
>>>>>>>
>>>>>>> Testing:
>>>>>>>     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
>>>>>>>     appcds tests on the above platforms + sparcv9.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>
>>>
>


More information about the hotspot-runtime-dev mailing list