file.cpp calls exit instead of throwing exception
#4938
Comments
|
I think this makes sense. My one concern is that in general we are trying to move to have less exception-handling code in Binaryen, in particular so that the wasm port can be built with |
|
Not using exceptions is generally better for rust too, so if there's a different error handling pattern you'd like I'm happy to do that instead. |
|
It doesn't seem like anyone else has strong opinions here, so I think using C++ exceptions for |
|
We have all kinds of error handling patterns in Binaryen. Another to consider would be something like I would mildly prefer any of those options to using exceptions, but also using exceptions here is certainly the simplest short-term fix. It would be nice to go through and standardize our error handling as a code health project at some point. |
|
Pulling the Result and MaybeResult types out of wat-parser.h and reusing them looks like something I can handle. I don't quite understand how to use BuildResult on first read though. |
|
After spending a day working on this I discovered the For my purposes, as an alternative to refactoring binaryen to propagate more non-exception errors, which will make the code increasingly more verbose, I could also convert the errors in file-io.cpp to use I know from the comments in |
|
Updating those exits to use |
…#5087) This patch makes binaryen easier to call from other applications by making more errors recoverable instead of early-exiting. The main thing it does is change three calls to exit on I/O errors into calls to Fatal(), which is an existing custom abstraction for handling unrecoverable errors. Currently Fatal's destructor calls _Exit(1). My intent is to make it possible for Fatal to not exit, but to throw, allowing an embedding application to catch the exception. Because the previous early exits were exiting with error code EXIT_FAILURE, I also changed Fatal to exit with EXIT_FAILURE. The test suite continues to pass so I assume this is ok. Next I changed Fatal to buffer its error message until the destructor instead of immediately printing it to stderr. This is for ease of patching Fatal to throw instead. Finally, I also included the patch I need to make Fatal throw when THROW_ON_FATAL is defined at compile time. I can carry this patch out of tree, but it is a small patch, so perhaps you will be willing to take it. I am happy to remove it. Fixes #4938
I am calling binaryen's ModuleReader and ModuleWriter APIs from Rust and discovered that there are three cases in file.cpp where it calls
exit. This isn't suitable for embedding in other applications. Can it be changed to throw an exception instead? Callers already need to deal with ParseException and MapParseException so this doesn't seem too burdensome a change.If it's ok to change this to throw exceptions, would following the same pattern as MapException be acceptable? I am willing to make this change myself.
The text was updated successfully, but these errors were encountered: