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

Remove name for unused arguments #672

Merged
merged 2 commits into from
Apr 14, 2017
Merged

Remove name for unused arguments #672

merged 2 commits into from
Apr 14, 2017

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Apr 13, 2017

with (void)arg; in the function's body.

@codecov-io
Copy link

codecov-io commented Apr 13, 2017

Codecov Report

Merging #672 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
- Coverage   92.91%   92.91%   -0.01%     
==========================================
  Files          65       65              
  Lines        3303     3302       -1     
==========================================
- Hits         3069     3068       -1     
  Misses        234      234
Impacted Files Coverage Δ
src/Rcpp_init.cpp 100% <ø> (ø) ⬆️
src/attributes.cpp 98.9% <100%> (-0.01%) ⬇️

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 21d8388...93dfa96. Read the comment docs.

@kevinushey
Copy link
Contributor

LGTM, although I wonder about a couple of the non-used arguments (it seems like they should likely be used?)

@kevinushey
Copy link
Contributor

Oh, it looks like a bunch of classes within attributes.cpp have the same interface but some don't need to handle the accepted arguments.

@jjallaire
Copy link
Member

jjallaire commented Apr 13, 2017 via email

@krlmlr krlmlr changed the title Annotate unused arguments Remove name for unused arguments Apr 14, 2017
@eddelbuettel
Copy link
Member

This one looks much better now too. Will pull it in, and then start another rev.dep. Don't expect anything though.

@eddelbuettel eddelbuettel merged commit e852135 into RcppCore:master Apr 14, 2017
@eddelbuettel
Copy link
Member

Woot. And just installed with ccache g++ -I/usr/share/R/include -DNDEBUG -I../inst/include/ -fpic -g -O3 -Wall -pipe -pedantic -Wextra -c. That's first.

Thanks for that. I am sure I will have to remove -Wextra from Makevars when compiling other packages but it is rather nice to have Rcpp squeaky clean.

@krlmlr krlmlr deleted the f-unused branch April 14, 2017 18:22
@krlmlr
Copy link
Contributor Author

krlmlr commented Apr 14, 2017

You're welcome. I tend to use a package-local Makevars (with a corresponding .Rprofile): krlmlr/scriptlets@8467934.

@eddelbuettel
Copy link
Member

eddelbuettel commented Apr 14, 2017

Not familiar with those hooks. Does it require devtools, or would standard R tools adhere to them too? In other words would Writing R Extensions describe this?

@krlmlr
Copy link
Contributor Author

krlmlr commented Apr 14, 2017

Works with base R, also in R CMD INSTALL. R_MAKEVARS_USER is described in R-admin.

@eddelbuettel
Copy link
Member

Nice! I'll follow up on that. I tend to be aware of what is in R-exts, but they trick me now by hiding more stuff in R-inst and R-admin.

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.

None yet

5 participants