-
Notifications
You must be signed in to change notification settings - Fork 699
Fix unchecked Errors #3097
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 unchecked Errors #3097
Conversation
01ef34a
to
6e6666c
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.
Looks like theres a deadlock, but couldn't see an obvious change that would cause it. I wonder if its an assert/error in another thread which shortcuts before a promise is set?
examples/mnist.cpp
Outdated
@@ -248,6 +248,7 @@ void testMNISTLoadAndTraining() { | |||
const char *inputName = "data"; | |||
|
|||
llvm::Error errPtr = llvm::Error::success(); | |||
(void)!!errPtr; // Mark Error as checked before it's assigned to. |
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.
So given this pattern is reused frequently in this diff can we make it a macro?
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.
I removed all but 3 of these (ones used for input to a Loader constructor) so now it's not too many and also I don't really want to make another macro for Error stuff when this is already a 1 liner
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.
The benefit of having macro is that it's very explicit, and no need for extra comments.
MARK_ERROR_CHECKED(errPtr);
no need to add comments.
f29e06a
to
6d4e48a
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.
LGTM
6d4e48a
to
383a9a0
Compare
383a9a0
to
2a43bc7
Compare
@@ -245,6 +245,7 @@ void testMNISTLoadAndTraining() { | |||
const char *inputName = "data"; | |||
|
|||
llvm::Error errPtr = llvm::Error::success(); | |||
MARK_ERR_CHECKED(errPtr); |
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.
Do you think it would be clearer/more concise to have a MAKE_SUCCESS() that magicks up a checked success()
value? (Would that even work?) Every where we use MARK_ERR_CHECKED
it seems like it's because we're creating an Error
to be possibly filled in later.
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.
I was thinking about how to do this but I can't think of one, can you?
@bertmaher I'm going to land this now to fix these errors but if we find a way to do MAKE_SUCCESS, I'll make a followup to use that instead. |
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.
@jackm321 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary:
llvm::Errors should always be checked and in debug mode will assert if they aren't checked.
There a several unchecked llvm::Errors in Glow, mostly in tests due to a workaround for MSVC which doesn't like
std::promise<llvm::Error>
sThis PR should fix the existing issues but we still need to setup CI to check this on an ongoing basis.
See #2737 for more details.
Documentation:
n/a