-
Notifications
You must be signed in to change notification settings - Fork 772
utl: ensure exception-safe file closure in stream and file handlers #9256
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
utl: ensure exception-safe file closure in stream and file handlers #9256
Conversation
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.
Code Review
This pull request effectively addresses the risk of exceptions being thrown from destructors in file handling classes by introducing explicit close() methods and ensuring destructors are exception-safe. This improves robustness and provides users a way to handle file closure errors explicitly. My review includes one high-severity suggestion for FileHandler::close() to ensure it properly reports errors instead of failing silently, which aligns with the overall goal of this pull request.
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.
clang-tidy made some suggestions
Addresses an issue where utl::OutStreamHandler could cause a crash (std::terminate) if a file flush or rename failed during destruction (e.g., due to ENOSPC). - Added explicit close() methods to OutStreamHandler, InStreamHandler, and FileHandler to allow users to handle closure errors explicitly. - Updated destructors to be exception-safe by catching potential errors during implicit closure. - Added diagnostic logging to std::cerr in destructors to report closure failures that would otherwise be suppressed. Fixes The-OpenROAD-Project#9252 Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
a737407 to
f76c87a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
oharboe
left a comment
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.
Nice! Ideally the close() is called from user-code so that the exception is propagated from user-code and not logged and silenced.
Addresses an issue where utl::OutStreamHandler could cause a crash (std::terminate) if a file flush or rename failed during destruction (e.g., due to ENOSPC).
Fixes #9252