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

Calvin Cheung calvin.cheung at oracle.com
Thu Jan 4 22:12:41 UTC 2018


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