RFR(S): 8170936: Logging: LogFileOutput.invalid_file_test crashes when executed twice.
thomas.stuefe at gmail.com
Tue Dec 20 20:20:07 UTC 2016
This looks fine now to me. Thanks for fixing!
Further comments inline.
On Tue, Dec 20, 2016 at 7:04 PM, Kirill Zhaldybin <
kirill.zhaldybin at oracle.com> wrote:
> Thank you for you comments. I really appreciate your review.
> Here are a new WebRev: http://cr.openjdk.java.net/~kz
> 1. renamed delete_directory to delete_empty_directory
> 2. left only GetLastError() in error output
> Could you please also read comments inline?
> Thank you.
> Regards, Kirill
> On 20.12.2016 16:13, Thomas Stüfe wrote:
>> (also, I am not a (R)eviewer, so I guess this does not count as real
>> On Tue, Dec 20, 2016 at 2:13 PM, Thomas Stüfe <thomas.stuefe at gmail.com
>> <mailto:thomas.stuefe at gmail.com>> wrote:
>> Hi Kirill,
>> thanks a lot for fixing this! This was an annoyance on Windows.
>> Just small remarks:
>> - RemoveDirectory will only work on empty directories. Recursively
>> deleting non-empty directories seems to be more difficult. So could
>> you please add a comment that this function only deletes empty
>> directories and is not intended to work recursively? Or rename the
>> function to delete_empty_directory()?
>> - You do not have to output errno if RemoveDirectory failed, that
>> number will be unrelated to the error. GetLastError should be enough.
>> I also wonder why all those utilitiy functions are imeplemented in a
>> header file? I think it would be better for a C file (can change
>> implementation without triggering rebuild; also binary may be
>> smaller, at least on slowdebug).
> well, I do not know why it was implemented that way but I could speculate
> that it was simpler (one file instead of two), the functions were not
> supposed to be changed often and side effects (size and build time
> increase) were not considered significant.
> But it is only my speculations :)
> If you think we should change this could you please let me know? I will
> file an RFE.
I do not have strong emotions, just small preferences :) I would leave it
up to Marcus, this is more of a style question.
Kind Regards, Thomas
>> Kind regards, Thomas
>> On Tue, Dec 20, 2016 at 1:56 PM, Kirill Zhaldybin
>> <kirill.zhaldybin at oracle.com <mailto:kirill.zhaldybin at oracle.com>>
>> Dear all,
>> Could you please review this fix for 8170936?
>> The issue was windows specific - remove() function failed to
>> delete a directory on that platform.
>> I added delete_directory function which uses winapi
>> RemoveDirectory function and also does GTest style check on its
>> I also changed TEST to TEST_VM on a few tests from the same file
>> since they used ResourceMark and crashed if they were run
>> CR: https://bugs.openjdk.java.net/browse/JDK-8170936
>> Thank you.
>> Regards, Kirill
More information about the hotspot-runtime-dev