RFR(S): 8170936: Logging: LogFileOutput.invalid_file_test crashes when executed twice.

[Kirill Zhaldybin]

Thomas,

Thank you for you comments. I really appreciate your review.
Here are a new WebRev: 
http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/webrev.01/

Changes:
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 review)
>
> 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()?
fixed
>     - You do not have to output errno if RemoveDirectory failed, that
>     number will be unrelated to the error. GetLastError should be enough.
fixed
>
>     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.
>
>     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>>
>     wrote:
>
>         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
>         result.
>         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 isolated.
>
>         WebRev:
>         http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/webrev.00/
>         <http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8170936/webrev.00/>
>         CR: https://bugs.openjdk.java.net/browse/JDK-8170936
>         <https://bugs.openjdk.java.net/browse/JDK-8170936>
>
>         Thank you.
>
>         Regards, Kirill
>
>
>