Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dll import/export visibility macro update #26

Merged

Conversation

kejxu
Copy link
Contributor

@kejxu kejxu commented Jan 18, 2019

further updates to support DLL import/export on Windows =)

  • add missing ROSCONSOLE_BACKEND_DECL (from previous visibility change) for console_backend.h
  • move console_impl functions shared by log4cxx, glog, print into one single header console_impl.h (and remove the function declarations from rosconsole.cpp)

as mentioned in the comments, the ROSCONSOLE_CONSOLE_IMPL_DECL macro is added to manage visibility for functions in the impl components in one centralized position. For the implementations (src/rosconsole/impl/rosconsole_xxx.cpp), ROSCONSOLE_CONSOLE_IMPL_EXPORTS needs to be defined in the .cpp file before including console_impl.h since these files are compiled to generate their own runtime binaries (dlls). Another way of achieving the same result would be to use independent visibility control macros for each implementation; however, that means more duplicated code and makes it harder to manage.

Another reason to use a centralized header is so that it'd be easier to notice when the declaration and definition have a mismatch (at compile time); otherwise, such mismatch would cause error at runtime, making it harder to spot.

johnsonshih and others added 5 commits January 18, 2019 14:36
* Add ROSCONSOLE_BACKEND_DECL for backend functions

* Add ROSCONSOLE_LOG4CXX_DECL for log4cxx interface functions
…os#2)

* Add ROSCONSOLE_BACKEND_DECL for backend functions

* Add ROSCONSOLE_LOG4CXX_DECL for log4cxx interface functions

* Extract the impl interface to a separate header for impl dll to use
@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit 6135fd3 into ros:melodic-devel Jan 26, 2019
@kejxu kejxu deleted the add_console_impl.h_for_visibility_control branch January 28, 2019 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants