Add option for enabling/disabling debug error output to cerr #75
Conversation
|
Don't quite understand what the problem on Windows is. Unfortunately, I don't have a windows toolchain set up so I cannot troubleshoot it currently. Any help would be very much appreciated. |
The problem is that you are defining a static global object in public headers, and then you are not exposing it appropriately to be visible. On Linux/macOS, everything works because the default visibility for this symbols is visible (see Section 2.2.2 of https://software.intel.com/sites/default/files/m/a/1/e/dsohowto.pdf for more info). To expose symbols in Windows shared library, we use the CMake property |
CMakeLists.txt
Outdated
| @@ -59,6 +59,11 @@ endif() | |||
| option(BUILD_TESTING "Create tests using CMake" OFF) | |||
| include(CTest) | |||
|
|
|||
| option(DEBUG_OUTPUT "Print debug error messages to cerr" ON) | |||
| if(DEBUG_OUTPUT) | |||
| add_compile_definitions("DEBUG_OUTPUT") | |||
There was a problem hiding this comment.
I think there may be two problems with this definition:
- It is not prefixed by any
OSQP_EIGEN_or similar prefix, so it could be prone to macro name collision, so probably it make sense to give to it a less generic name. - To avoid confusion for which target this compile definition is valid, I would suggest to use
target_compile_definitionsinstead ofadd_compile_definitions
src/Debug.cpp
Outdated
| class NullStream : public std::ostream { | ||
| public: | ||
| NullStream() : std::ostream(nullptr) {} | ||
| NullStream(const NullStream &) : std::ostream(nullptr) {} | ||
| }; |
There was a problem hiding this comment.
This definition will pollute the global symbols of the osqp-eigen library with a symbol that is not in the OsqpEigen namespace, making it prone to collisions. Could it make sense to move this in the OsqpEigen namespace?
To clarify, probably the easiest solution is just to have a regular function that returns a reference to the |
|
Furthermore, can you please accept the CLA in https://cla-assistant.io/robotology/idyntree?pullRequest=700 ? Sorry for the request, but it is for internal policies. |
Done! |
|
@traversaro & @S-Dafarra Thanks for your suggestions and comments. I will prepare a fix. |
|
The pushed commit contains fixes to the issues highlighted. I agree that a function |
kalj
force-pushed
the
allow-disabling-debug-output
branch
from
345ba23 to
1c6ae06
Compare
|
Just noted that I had tabs instead of spaces, and fixed it. |
|
Are we missing something here? |
There was a problem hiding this comment.
LGTM, thanks! If @traversaro and @GiulioRomualdi agree we can move on to the merge for me.
Note that depending on the notifications settings, it is possible that reviewers do not get notified if you push commits to a PR, so if you did not received feedback it may be due to that. |
|
@kalj thank you for the contribution |
Possible fix of #74.