-
Notifications
You must be signed in to change notification settings - Fork 338
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
fix rename or remove #2276
fix rename or remove #2276
Conversation
I agree that it's a bit too close to the C error code mechanism. |
std::error_code ec2; | ||
fs::remove(to, ec2); |
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.
Not so familiar with error code, but shouldn't we not checking the result here?
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.
we don't really care about the result because at this point the operation has already failed.
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.
Adding [[maybe_unused]]
here would clarify that and avoid warnings.
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.
LGTM
I wish it was testable in some ways...
I remember people in Boost and C++ committee agreeing with you (and adding some other issues with |
This fixes the usage of the confusing
ec
boolean values :)It would be much nicer if the
error_code
had ahas_error()
function to make it easier to understand what we're testing.Maybe @JohanMabille or @Klaim can give this a review to make sure it's correct this time around.