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

Add Shield around parameters in Rcpp::interfaces (fixes #712) #713

Merged
merged 1 commit into from Jun 14, 2017

Conversation

Projects
None yet
6 participants
@jjallaire
Member

jjallaire commented Jun 14, 2017

No description provided.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 14, 2017

Member

Hah. Saves me some testing and means I can go straight to rev.dep testing this :)

Member

eddelbuettel commented Jun 14, 2017

Hah. Saves me some testing and means I can go straight to rev.dep testing this :)

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 14, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #713   +/-   ##
=======================================
  Coverage   89.57%   89.57%           
=======================================
  Files          66       66           
  Lines        3588     3588           
=======================================
  Hits         3214     3214           
  Misses        374      374
Impacted Files Coverage Δ
src/attributes.cpp 98.4% <ø> (ø) ⬆️

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 c75bbdd...e7b1698. Read the comment docs.

codecov-io commented Jun 14, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #713   +/-   ##
=======================================
  Coverage   89.57%   89.57%           
=======================================
  Files          66       66           
  Lines        3588     3588           
=======================================
  Hits         3214     3214           
  Misses        374      374
Impacted Files Coverage Δ
src/attributes.cpp 98.4% <ø> (ø) ⬆️

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 c75bbdd...e7b1698. Read the comment docs.

@@ -2172,7 +2172,7 @@ namespace attributes {
const std::vector<Argument>& args = function.arguments();
for (std::size_t i = 0; i<args.size(); i++) {
ostr() << "Rcpp::wrap(" << args[i].name() << ")";
ostr() << "Shield<SEXP>(Rcpp::wrap(" << args[i].name() << "))";

This comment has been minimized.

@eddelbuettel

eddelbuettel Jun 14, 2017

Member

We could make this Rcpp::Shield for consistency with Rcpp::wrap but I see other identifiers without the namespace qualifier.

@eddelbuettel

eddelbuettel Jun 14, 2017

Member

We could make this Rcpp::Shield for consistency with Rcpp::wrap but I see other identifiers without the namespace qualifier.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Jun 14, 2017

Member
Member

jjallaire commented Jun 14, 2017

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 14, 2017

Member

Looks good, and have a rev.dep run chugging along. May as well merge now.

Member

eddelbuettel commented Jun 14, 2017

Looks good, and have a rev.dep run chugging along. May as well merge now.

@eddelbuettel eddelbuettel merged commit 1ec267a into master Jun 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Jun 14, 2017

Contributor

Would it be worth it to prefix with Rcpp:: where applicable to avoid any namespace headaches?

Contributor

coatless commented Jun 14, 2017

Would it be worth it to prefix with Rcpp:: where applicable to avoid any namespace headaches?

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Jun 15, 2017

Contributor

The fact that we're injecting a using namespace Rcpp; statement into another package's namespace within a header file is a little bit wonky, so it might be worth doing.

Contributor

kevinushey commented Jun 15, 2017

The fact that we're injecting a using namespace Rcpp; statement into another package's namespace within a header file is a little bit wonky, so it might be worth doing.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Jun 15, 2017

Member
Member

jjallaire commented Jun 15, 2017

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 15, 2017

Member

Also, aren't we talking RcppExports.cpp here -- a single compilation unit and not included into other files.

Member

eddelbuettel commented Jun 15, 2017

Also, aren't we talking RcppExports.cpp here -- a single compilation unit and not included into other files.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Jun 15, 2017

Contributor

Good point :) Okay, let's not touch anything there.

Contributor

kevinushey commented Jun 15, 2017

Good point :) Okay, let's not touch anything there.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 16, 2017

Contributor

Thanks for the very quick fix!

Contributor

krlmlr commented Jun 16, 2017

Thanks for the very quick fix!

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 16, 2017

Member

Thanks to you for the heads-up. I also ran a full rev.dep check yesterday, and we're good. I did sixteen out of 1050 or so packages failing, and about eight looked like possible dplyr interactions I had not seen (and do no worry about for Rcpp as they also fail at CRAN for the released version -- hence unlikely to be us).
See RcppCore/rcpp-logs@1f85c64

Member

eddelbuettel commented Jun 16, 2017

Thanks to you for the heads-up. I also ran a full rev.dep check yesterday, and we're good. I did sixteen out of 1050 or so packages failing, and about eight looked like possible dplyr interactions I had not seen (and do no worry about for Rcpp as they also fail at CRAN for the released version -- hence unlikely to be us).
See RcppCore/rcpp-logs@1f85c64

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 16, 2017

Contributor

Thanks. Most of the eight new failures look familiar, I hope the upcoming dplyr point release will fix them.

Contributor

krlmlr commented Jun 16, 2017

Thanks. Most of the eight new failures look familiar, I hope the upcoming dplyr point release will fix them.

@eddelbuettel eddelbuettel deleted the bugfix/interfaces-param-shield branch Jun 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment