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

[Ioi Lam]

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