-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
removeFile() Usage #1424
removeFile() Usage #1424
Conversation
src/axom/slic/tests/slic_macros.cpp
Outdated
// Closes open file streams associated with Slic streams when destructors | ||
// called during slic::finalize(). | ||
// Windows _unlink file deletion fails if file is still in use. | ||
#ifdef WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a reason non-Windows should not finalize(), let's please remove the guard on WIN32
here and in the slic_macros_parallel.cpp test. If there is a reason to test removeFile() on non-finaliz()ed slic on non-Windows platforms, I would appreciate knowing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard removed! finalize()
call will work on Windows and non-Windows systems, guard was a relic of my testing.
@bmhan12 , thank you for your attention to detail on this. |
fdc0fed
to
e528819
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bmhan12 !
@@ -457,7 +456,7 @@ TEST(slic_macros, test_macros_file_output) | |||
std::string no_fmt_expected; | |||
no_fmt_expected += "*****\n[INFO]\n\n Test \n\n "; | |||
no_fmt_expected += __FILE__; | |||
no_fmt_expected += "\n441\n****\n"; | |||
no_fmt_expected += "\n440\n****\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is 440
here? Is it a line number?
It might make sense to add a comment about this for future maintenance of these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is 440 here? Is it a line number?
Yes. In a follow-up PR related to #1410, I'll be updating these line numbers to be like slic_macros_parallel
to be __LINE__ - offset
to make that clear.
@bmhan12 -- could you please update the documentation for See: axom/src/axom/core/utilities/FileUtilities.hpp Lines 86 to 92 in 24751dd
|
This PR:
removeFile
failure on Windows inslic_macros.cpp
unit test by closing the files associated withifstream
and theofstream
member variables ofLogStream
instances.removeFile
in unit tests to verify file has been deletedCloses issue #1415