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

explicitly make exported functions visible #720

Merged
merged 1 commit into from Jun 28, 2017

Conversation

Projects
None yet
4 participants
@jeroen
Contributor

jeroen commented Jun 27, 2017

Marks exported Rcpp functions as visible, on supported platforms. The attribute_visible macro is defined in <R_ext/Visibility.h>. If the compiler supports visibility it is defined as

 __attribute__ ((visibility ("default")))

And otherwise it is blank. The idea is that we can compile packages with -fvisibility=hidden as defined in make variable $(C_VISIBILITY) on supported platforms. So pkg authors can do:

PKG_CXXFLAGS=$(C_VISIBILITY)

Limiting visibility to exported functions reduces the chances of symbol conflicts like we saw in ropensci/hunspell#21. Here both rstudio and the hunspell package include a copy of libhunspell but different versions. Apparently their C++ classes start conflicting under certain circumstances. Hiding the libhunspell symbols from the dll solved the problem.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 27, 2017

Codecov Report

Merging #720 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #720   +/-   ##
=======================================
  Coverage   93.15%   93.15%           
=======================================
  Files          66       66           
  Lines        3432     3432           
=======================================
  Hits         3197     3197           
  Misses        235      235
Impacted Files Coverage Δ
inst/include/RcppCommon.h 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ca4f6a...f873b97. Read the comment docs.

codecov-io commented Jun 27, 2017

Codecov Report

Merging #720 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #720   +/-   ##
=======================================
  Coverage   93.15%   93.15%           
=======================================
  Files          66       66           
  Lines        3432     3432           
=======================================
  Hits         3197     3197           
  Misses        235      235
Impacted Files Coverage Δ
inst/include/RcppCommon.h 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ca4f6a...f873b97. Read the comment docs.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 28, 2017

Member

Interesting. I was so far only aware of a user-space clash between Boost and RStudio which I had hit with anytime and (the new, non CRAN) RcppMLPACK. This got fixed in the RStudio dailies a little while ago thanks to @kevinushey

I am always a little wary of making a global change but this one may be sensible. Maybe to wait till I get home though.

Alternatively could we change this to defining and inly if another variable is defined? I seem to have

C_VISIBILITY = -fvisibility=hidden
F77_VISIBILITY = -fvisibility=hiddenb

so should we condition on that?

Member

eddelbuettel commented Jun 28, 2017

Interesting. I was so far only aware of a user-space clash between Boost and RStudio which I had hit with anytime and (the new, non CRAN) RcppMLPACK. This got fixed in the RStudio dailies a little while ago thanks to @kevinushey

I am always a little wary of making a global change but this one may be sensible. Maybe to wait till I get home though.

Alternatively could we change this to defining and inly if another variable is defined? I seem to have

C_VISIBILITY = -fvisibility=hidden
F77_VISIBILITY = -fvisibility=hiddenb

so should we condition on that?

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Jun 28, 2017

Contributor

I think the conditioning is already done by R itself. Header R_ext/Visibility.h has:

#ifdef HAVE_VISIBILITY_ATTRIBUTE
# define attribute_visible __attribute__ ((visibility ("default")))
# define attribute_hidden __attribute__ ((visibility ("hidden")))
#else
# define attribute_visible
# define attribute_hidden
#endif

Where HAVE_VISIBILITY_ATTRIBUTE is defined in Rconfig.h if the compiler supports it. When HAVE_VISIBILITY_ATTRIBUTE is not defined, both C_VISIBILITY and attribute_visible are blank.

Contributor

jeroen commented Jun 28, 2017

I think the conditioning is already done by R itself. Header R_ext/Visibility.h has:

#ifdef HAVE_VISIBILITY_ATTRIBUTE
# define attribute_visible __attribute__ ((visibility ("default")))
# define attribute_hidden __attribute__ ((visibility ("hidden")))
#else
# define attribute_visible
# define attribute_hidden
#endif

Where HAVE_VISIBILITY_ATTRIBUTE is defined in Rconfig.h if the compiler supports it. When HAVE_VISIBILITY_ATTRIBUTE is not defined, both C_VISIBILITY and attribute_visible are blank.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 28, 2017

Member

Thanks for that, very helpful. I very vaguely recalled it but did not have time to hunt.

Member

eddelbuettel commented Jun 28, 2017

Thanks for that, very helpful. I very vaguely recalled it but did not have time to hunt.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Jun 28, 2017

Member

Seems like a good change to me, I'll merge it. I can't forsee any problems with revdeps but we'll see on the next pass!

Member

jjallaire commented Jun 28, 2017

Seems like a good change to me, I'll merge it. I can't forsee any problems with revdeps but we'll see on the next pass!

@jjallaire jjallaire merged commit 7d1973c into RcppCore:master Jun 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment