Comments (10)
Thanks, but I think the default shall not be aborting. It should be throwing an exception for example, which would crash anyway if it's not catched.
By the way, encountering NaN is hardly a situation for aborting a program, NaNs handling depends a lot on the application.
So it boils down to Sophus providing better defaults:
- in debug, throw an exception or trigger a breakpoint
- in release, at most print the error, but do not crash
from sophus.
@TLescoatTFX Thanks for your suggestions. After checking openCV, I think your suggestion is quite reasonable.
Here is what openCV do:
https://github.com/opencv/opencv/blob/853e5dfcdf091023b78283d6dc03d91fd6763495/modules/core/doc/intro.markdown?plain=1#L296-L298
#CV_Assert(condition) macro that checks the condition and throws an exception when it is not
satisfied. For performance-critical code, there is #CV_DbgAssert(condition) that is only retained in
the Debug configuration.
Interestingly, I also noticed that abort
is used in the definition of those macros specifically for static analyzer. I don't know why. Here is the relevant code:
https://github.com/opencv/opencv/blob/853e5dfcdf091023b78283d6dc03d91fd6763495/modules/core/include/opencv2/core/base.hpp#L300-L306
#ifdef CV_STATIC_ANALYSIS
// In practice, some macro are not processed correctly (noreturn is not detected).
// We need to use simplified definition for them.
#define CV_Error(code, msg) do { (void)(code); (void)(msg); abort(); } while (0)
#define CV_Error_(code, args) do { (void)(code); (void)(cv::format args); abort(); } while (0)
#define CV_Assert( expr ) do { if (!(expr)) abort(); } while (0)
from sophus.
Hard disagree:
- NaN are not UB, from my understanding it is implementation-defined (but everyone use IEEE754).
- abort() gets in the way of debuggers, while also hiding the error. This is not just terrible, this is the worst possible way to report an error. If you have a bit of respect for your user's time, use OpenCV behavior please.
- Doing anything when preconditions are violated is not a C++ thing, this tautology is obviously true for other languages. But most libraries will kindly report an error and not crash in the presence of a minor error...
Anyway, that's your library, you do what you want, but fixing that faulty behavior would definitely be an improvement
from sophus.
SOPHUS_ENSURE
can be turned off by setting SOPHUS_DISABLE_ENSURES
, see
Line 101 in d270df2
In principle, one can enable SOPHUS_ENSURE
in debug mode for safety and turn it off in release mode for performance by cmake.
Hope this can help.
from sophus.
I'm glad you find such suggestion reasonable ! The behavior of OpenCV seems also good, I think they put the abort()
so that the static analysis understands better that the condition in assert shall always be true (a workaround, maybe ?)
from sophus.
A library shall never ever call std::abort(), std::terminate(), std::exit(),
Well, in c++ libraries can do anything when preconditions are violated. In my opinion, calling abort is much better than UB.
from sophus.
Hard disagree:
* NaN are _not_ UB, from my understanding it is implementation-defined (but everyone use IEEE754). * abort() gets in the way of debuggers, while also hiding the error. This is not just terrible, this is the worst possible way to report an error. If you have a bit of respect for your user's time, use OpenCV behavior please. * Doing anything when preconditions are violated is not a C++ thing, this tautology is obviously true for other languages. But most libraries will kindly report an error and not crash in the presence of a minor error...
Anyway, that's your library, you do what you want, but fixing that faulty behavior would definitely be an improvement
Yeah, agree~ "The program has unexpectedly finished." with no reason is pretty hard for the user to debug.
from sophus.
@TLescoatTFX, thanks for you candid feedback. So let me elaborate.
In sophus, NAN's on input variables are undocumented (I have to admit) precondition violations. When I will find some time, I make these preconditions more explicit in the comments.
On on precondition violations, the library if free to do anything, and SOPHUS chooses to call std::abort by default.
- abort() gets in the way of debuggers, while also hiding the error. This is not just terrible, this is the worst possible way to report an error. If you have a bit of respect for your user's time, use OpenCV behavior please.
I know there are different schools of thoughts, but in my workflow (as well as the workflows of the companies I worked at), it is actually the opposite. In this case, I will get an error message with a line number of the root of the error without the need to run it with a debugger. And when running it with the debugger, I still get a full stack-trace with std::abort (but not sure this is specific to my setup somehow...).
I also believe that it is the opposite of hiding an error. In most cases, such precondition violations, NANs or not normalized quaternions inputs are often caused by bugs in parsing code or numeric optimization routines. Throw an exception or side-stepping the case (e.g. by re-normalizing the quaternion silently), is actually hiding those non-trivial problems, and failing on a precondition violation makes them explicit, and causing a runtime abort often very close to the actual root cause.
I speaking from experience here (on large production mono-repos).
(A future API extension - following my opinionated design - could offer various tryGetFoo(...)
overloads for GetFoo(...)
which would return an std::expected<...>
type. This way you can have an API where the set of preconditions is reduced.)
But, I know that there is a point of view where folks do not like std::abort on precondition violations. That's why there is the custom SOPHUS_ENABLE_ENSURE_HANDLER where you can hook in your own error handling (e.g. OpenCV's).
Having that said, I have not used SOPHUS_ENABLE_ENSURE_HANDLER and similar macros in a while - since I'm mainly working on sophus2 and sophus-rs.
So please, @TLescoatTFX, if you have some PRs to make this easier to plug in your own error handling system, I'm more than happy to accept them (and if CI passes). And please ping me since I'm not that often review the Sophus (1) repo anymore.
from sophus.
abort() gets in the way of debuggers, while also hiding the error.
I just tried it out and I'm getting this stack-trace in GDB with the corresponding line number (compiled with RelWithDebInfo):
Sophus ensure failed in function 'int main()', file '/home/strasdat/dev/Sophus/examples/HelloSO3.cpp', line 31.
Message: 33
Program received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=140737352537920) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737352537920) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=140737352537920) at ./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=140737352537920, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3 0x00007ffff7842476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4 0x00007ffff78287f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x000055555555793b in main () at /home/strasdat/dev/Sophus/examples/HelloSO3.cpp:31
@TLescoatTFX what is you concrete OS and build config?
from sophus.
The PR above includes an example of how to use a custom SOPHUS_ENABLE_ENSURE_HANDLER and hence throw an exception instead of calling std::abort:
Sophus/examples/CMakeLists.txt
Line 16 in ae3a904
from sophus.
Related Issues (20)
- Broken link on landing page README.rst HOT 4
- Compilation Error on Raspberry Pi 4 Buster HOT 2
- Looks like 22.10 and 22.04.1 tags got removed? HOT 3
- How to creating a Sophus::SE3f variable from a Eigen::Matrixf variable? HOT 4
- install erorr issue HOT 1
- Wrong compiler standard in CMakeLists.txt HOT 2
- Documentation link is broken HOT 3
- SOPHUS_USE_BASIC_LOGGING=ON compilation error HOT 1
- rotationMatrix() and angles give different results HOT 4
- FMT warning when not found HOT 1
- CUDA::cublas but the target was not found HOT 1
- Sophus ensure failed in function ..... HOT 2
- Error on building ORB-Slam3 on Debian-12 x86-64 HOT 1
- sophus HOT 2
- Cannot compile sophus_pybind in Ubuntu 20.04 HOT 6
- Add CI for CUDA HOT 1
- Remove `CMAKE_<LANG>_FLAGS`` from CMakeLists.txt
- 404 for site `https://strasdat.github.io/Sophus/latest/` HOT 5
- Build Error HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from sophus.