search

robotology / osqp-eigen

Simple Eigen-C++ wrapper for OSQP library
https://robotology.github.io/osqp-eigen/
BSD 3-Clause "New" or "Revised" License
385 stars 114 forks source link

Add option for enabling/disabling debug error output to cerr #75

Closed kalj closed 3 years ago

kalj commented 3 years ago

Possible fix of #74.

kalj commented 3 years ago

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.

traversaro commented 3 years ago

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 WINDOWS_EXPORT_ALL_SYMBOLS, that however does not work with global variables, see https://blog.kitware.com/create-dlls-on-windows-without-declspec-using-new-cmake-export-all-feature/ and https://github.com/robotology/how-to-export-cpp-library/issues/41 . However, looking a bit more in the PR I am afraid there some preliminary problems, I will comment inline.

traversaro commented 3 years ago

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 WINDOWS_EXPORT_ALL_SYMBOLS, that however does not work with global variables, see https://blog.kitware.com/create-dlls-on-windows-without-declspec-using-new-cmake-export-all-feature/ and robotology/how-to-export-cpp-library#41 . However, looking a bit more in the PR I am afraid there some preliminary problems, I will comment inline.

To clarify, probably the easiest solution is just to have a regular function that returns a reference to the std::ostream &, instead of using a global variable. Using a function instead of a global variable is also more convenient from the point of view of ABI maintenance.

traversaro commented 3 years ago

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.

kalj commented 3 years ago

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!

kalj commented 3 years ago

@traversaro & @S-Dafarra Thanks for your suggestions and comments. I will prepare a fix.

kalj commented 3 years ago

The pushed commit contains fixes to the issues highlighted. I agree that a function std::ostream& debugStream() is a better solution in all regards..

kalj commented 3 years ago

Just noted that I had tabs instead of spaces, and fixed it.

kalj commented 3 years ago

Are we missing something here?

traversaro commented 3 years ago

Are we missing something here?

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.

GiulioRomualdi commented 3 years ago

@kalj thank you for the contribution