Create glow::Error and glow::Expected #3515
Conversation
jackm321
force-pushed
the
glow_error
branch
3 times, most recently
from
c89e06b to
45c422c
Compare
There was a problem hiding this comment.
@jackm321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
jackm321
force-pushed
the
glow_error
branch
from
45c422c to
b8ed216
Compare
There was a problem hiding this comment.
@jackm321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
jackm321
force-pushed
the
glow_error
branch
from
b8ed216 to
2e73928
Compare
lib/Support/Error.cpp
Outdated
| return ss.str(); | ||
| } | ||
|
|
||
| // static |
There was a problem hiding this comment.
Do you plan to remove this static?
| #include <glog/logging.h> | ||
|
|
||
| /// NOTE: please only use code and macros that resides outside of the detail |
There was a problem hiding this comment.
Are most of the changes in this file "inspired" by LLVM's Error.h, i.e. is it pretty-much a copy of the LLVM's code with minor changes? Is there any possibility to change LLVM's behavior without literally re-defining all these types? Probably not. But it is annoying to have so much boilerplate code ;-(
There was a problem hiding this comment.
@opti-mix yeah it is overall pretty similar to LLVM's Error and Expected with a changes like abstracting out the state checking. The thing that it does is pretty dead simple so there isn't a lot of room for creative implementations.
As I mentioned in person, I would actually like to design this a bit differently, have only Expected and model the Error use case with a template specialization for Expected so we only have a single type everywhere. However for the sake of sanity, I think we should switch to a near drop-in replacement first and then consider later whether we want to go in a different direction like this instead of attempting to do everything at once.
Because llvm::Error's checking is disabled at compile time I don't think there is a clean and easy way to turn this back on by wrapping it externally and removing our dependency on llvm::Errors allows us the freedom to change things later as mentioned above.
|
@opti-mix Thank you for reviewing this monster, really appreciate it! |
| namespace detail { | ||
| /// enableCheckingErrors is used to enable assertions that every Error and | ||
| /// Expected has its status checked before it is destroyed. This should be | ||
| /// enabled in debug builds but turned off otherwise. |
There was a problem hiding this comment.
BTW, a quick question: What if this could be controlled by a command-line flag instead? It would be disabled by default in release builds, but one could enable it via such a command-line option if needed? Would it bring any value?
There was a problem hiding this comment.
That would be worse because when it is disabled at build time
- Error and Expected use less space because no need to store
CheckState:: checked_ - The code could be slightly faster because the code to do the checking is completely non-existent so no need even to check a flag to see if the state should be checked.
Also this is really a debugging mechanism so I think it makes sense to enable it in only debug mode. My view on it is that ideally, ensuring that Errors are always checked would be done statically but this isn't Rust so having some debug assertions is probably the next best thing, but we shouldn't be putting that check into Release. What do you think?
There was a problem hiding this comment.
Yeah, makes sense. Thanks for the explanation!
jackm321
force-pushed
the
glow_error
branch
2 times, most recently
from
4cd6a85 to
73df2f1
Compare
There was a problem hiding this comment.
@jackm321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: _Note for review: Most real changes are in `Error.h`, `Error.cpp`, and `ErrorTest.cpp` and the rest is mostly codemod to switch from `llvm::Error/Expected` to `glow::Error/Expected`._ Removes dependency on `llvm::Error` and ensures Errors are don't go unchecked by verifying this in debug build. `glow::Error/Expected` are mostly drop-in replacements for `llvm::Error/Expected` but exclude some features not used in Glow such as multiple error value types, reference types contained in Expected, etc. They also have a couple of different features like `Error::empty()` which creates "pre-checked" `Error`s. Documentation: doxygen fixes: pytorch#2737, pytorch#3505 Pull Request resolved: pytorch#3515 Test Plan: added `ErrorTest` unit tests ninja check Differential Revision: D17399297 fbshipit-source-id: 4b0a524e2c1ba2cf4adc766a0e8196ee94e062f0
jackm321
force-pushed
the
glow_error
branch
from
73df2f1 to
5d356c8
Compare
Note for review: Most real changes are in
Error.h,Error.cpp, andErrorTest.cppand the rest is mostly codemod to switch fromllvm::Error/Expectedtoglow::Error/Expected.Summary:
Removes dependency on
llvm::Errorand ensures Errors are don't go unchecked by verifying this in debug build.glow::Error/Expectedare mostly drop-in replacements forllvm::Error/Expectedbut exclude some features not used in Glow such as multiple error value types, reference types contained in Expected, etc. They also have a couple of different features likeError::empty()which creates "pre-checked"Errors.Documentation:
doxygen
fixes: #2737, #3505
Test Plan:
added
ErrorTestunit testsninja check