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

BEGIN_RCPP uses the variables it defines #671

Merged
merged 2 commits into from Apr 14, 2017

Conversation

Projects
None yet
5 participants
@krlmlr
Contributor

krlmlr commented Apr 13, 2017

Will be used instead of BEGIN_RCPP for generated wrappers for C++ interfaces ([[Rcpp::interfaces(cpp)]]).

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 13, 2017

Member

This one looks, quite frankly, suspicious. Or maybe I need my 2nd cup of coffee for the morning.

Under a certain conditions you add an opening brace as well as try. I see no matching closing brace. Hm.

Member

eddelbuettel commented Apr 13, 2017

This one looks, quite frankly, suspicious. Or maybe I need my 2nd cup of coffee for the morning.

Under a certain conditions you add an opening brace as well as try. I see no matching closing brace. Hm.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
Contributor

krlmlr commented Apr 13, 2017

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 13, 2017

Member

Ack.

The name sucks. Maybe BEGIN_RCPP_NO_VARS ?

And what did your mother tell you about opening issues tickets before PRs? I know you don;t believe me yet that "Suggests ! Depend" (but you will ;-) ) but also PR != Issue. ;-)

Member

eddelbuettel commented Apr 13, 2017

Ack.

The name sucks. Maybe BEGIN_RCPP_NO_VARS ?

And what did your mother tell you about opening issues tickets before PRs? I know you don;t believe me yet that "Suggests ! Depend" (but you will ;-) ) but also PR != Issue. ;-)

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Apr 13, 2017

Codecov Report

Merging #671 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   92.91%   93.19%   +0.27%     
==========================================
  Files          65       65              
  Lines        3303     3466     +163     
==========================================
+ Hits         3069     3230     +161     
- Misses        234      236       +2
Impacted Files Coverage Δ
src/attributes.cpp 98.89% <100%> (-0.02%) ⬇️

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

codecov-io commented Apr 13, 2017

Codecov Report

Merging #671 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   92.91%   93.19%   +0.27%     
==========================================
  Files          65       65              
  Lines        3303     3466     +163     
==========================================
+ Hits         3069     3230     +161     
- Misses        234      236       +2
Impacted Files Coverage Δ
src/attributes.cpp 98.89% <100%> (-0.02%) ⬇️

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

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 13, 2017

Member

And of course, this one is basically untestable by reverse-depends R CMD check as that tickles compilations and checks almost certainly almost none of which will involve a call to compileAttributes().

But it seems fine on the merits.

Member

eddelbuettel commented Apr 13, 2017

And of course, this one is basically untestable by reverse-depends R CMD check as that tickles compilations and checks almost certainly almost none of which will involve a call to compileAttributes().

But it seems fine on the merits.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 13, 2017

Contributor

The BEGIN_RCPP_RETURN_ERROR is a companion to END_RCPP_RETURN_ERROR, both are relatively close to each other in the generated code. But I can change it to use any name you prefer.

Testing it involves using it in a package; maybe a bunch of acceptance/integration tests in Rcpp that just compile an Rcpp package will be helpful? Gábor has created a few utility packages that will make this a pleasant exercise.

A patch is worth a thousand words (and is faster to write, too).

Contributor

krlmlr commented Apr 13, 2017

The BEGIN_RCPP_RETURN_ERROR is a companion to END_RCPP_RETURN_ERROR, both are relatively close to each other in the generated code. But I can change it to use any name you prefer.

Testing it involves using it in a package; maybe a bunch of acceptance/integration tests in Rcpp that just compile an Rcpp package will be helpful? Gábor has created a few utility packages that will make this a pleasant exercise.

A patch is worth a thousand words (and is faster to write, too).

@krlmlr krlmlr referenced this pull request Apr 13, 2017

Merged

Create Makevars on Travis #2642

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Apr 13, 2017

Contributor

Looks good to me!

Contributor

kevinushey commented Apr 13, 2017

Looks good to me!

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Apr 13, 2017

Member

LGTM as well.

Member

jjallaire commented Apr 13, 2017

LGTM as well.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 13, 2017

Member

Yes, this one is simple enough. I clearly misread when I just got up.

I think I'd still like to rename it.

Member

eddelbuettel commented Apr 13, 2017

Yes, this one is simple enough. I clearly misread when I just got up.

I think I'd still like to rename it.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Apr 13, 2017

Member
Member

jjallaire commented Apr 13, 2017

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Apr 13, 2017

Member
Member

jjallaire commented Apr 13, 2017

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 13, 2017

Member

But it does nothing w.r.t. to error handlers or triggers. Which is why I find putting 'ERROR' in there a bit weird.

Otherwise of course on board re the symmetry.

Member

eddelbuettel commented Apr 13, 2017

But it does nothing w.r.t. to error handlers or triggers. Which is why I find putting 'ERROR' in there a bit weird.

Otherwise of course on board re the symmetry.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Apr 13, 2017

Member

The second macro does though (it returns an error rather than throwing one to workaround the win32 can't throw exceptions across modules when static linking to gcc runtime issue). So "RETURN_ERROR" really just means "this code will return an error rather than throwing one".

I know you are already aware of all that, just trying to elaborate on why I think the macro name is acceptable as an enclosing "concept" rather than literally what the individual macro does.

Member

jjallaire commented Apr 13, 2017

The second macro does though (it returns an error rather than throwing one to workaround the win32 can't throw exceptions across modules when static linking to gcc runtime issue). So "RETURN_ERROR" really just means "this code will return an error rather than throwing one".

I know you are already aware of all that, just trying to elaborate on why I think the macro name is acceptable as an enclosing "concept" rather than literally what the individual macro does.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 13, 2017

Contributor

One thing I haven't considered: packages that are using the new macro won't work with an older Rcpp version. What's the best way to ensure this? Should we patch DESCRIPTION in compileAttributes()?

Contributor

krlmlr commented Apr 13, 2017

One thing I haven't considered: packages that are using the new macro won't work with an older Rcpp version. What's the best way to ensure this? Should we patch DESCRIPTION in compileAttributes()?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 13, 2017

Member

No, we will not overwrite every user's DESCRIPTION file. We're not roxygen2 or devtools or other rude packages.

The new macro is one of a large number of entirely optional features within the package, and when maintainers Alice or Bob use it, they are expected to edit their DESCRIPTION files accordingly. So a simple comment next to the definition (ie // requires Rcpp (>= 0.12.1)) should do.

Or concretely, if an when you use it in dplyr, just change its DESCRIPTION file at that point.

Member

eddelbuettel commented Apr 13, 2017

No, we will not overwrite every user's DESCRIPTION file. We're not roxygen2 or devtools or other rude packages.

The new macro is one of a large number of entirely optional features within the package, and when maintainers Alice or Bob use it, they are expected to edit their DESCRIPTION files accordingly. So a simple comment next to the definition (ie // requires Rcpp (>= 0.12.1)) should do.

Or concretely, if an when you use it in dplyr, just change its DESCRIPTION file at that point.

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Apr 13, 2017

Member
Member

jjallaire commented Apr 13, 2017

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 13, 2017

Contributor

The last commit might work better (doesn't require changes in generated code), but I need to verify that it's actually helpful.

Contributor

krlmlr commented Apr 13, 2017

The last commit might work better (doesn't require changes in generated code), but I need to verify that it's actually helpful.

@krlmlr krlmlr changed the title from New BEGIN_RCPP_RETURN_ERROR that doesn't define unused variables to BEGIN_RCPP uses the variables it defines Apr 14, 2017

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 14, 2017

Contributor

Confirmed that the proposed solution actually clears the "unused variable" warnings.

Contributor

krlmlr commented Apr 14, 2017

Confirmed that the proposed solution actually clears the "unused variable" warnings.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 14, 2017

Member

Reverse-depends check worked just fine, so merging this.

Proposed change is also nice and shorter. Good.

Member

eddelbuettel commented Apr 14, 2017

Reverse-depends check worked just fine, so merging this.

Proposed change is also nice and shorter. Good.

@eddelbuettel eddelbuettel merged commit e1ef61d into RcppCore:master Apr 14, 2017

1 check passed

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

@krlmlr krlmlr deleted the krlmlr:f-cpp-interfaces-unused branch Apr 14, 2017

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 14, 2017

Contributor

Awesome, thanks! Are you planning to release mid-May?

Contributor

krlmlr commented Apr 14, 2017

Awesome, thanks! Are you planning to release mid-May?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 14, 2017

Member

Yes, I see no reason to depart from the bi-monthly schedule (apart from having a very busy May myself, but we'll cope). Any plans/concerns regarding the timeline?

I did of course skip one your packages in testing, but we've been down that road before ;-)

Also just added ChangeLog and NEWS. Alter as needed in next PR and keep in mind that Contributing.md suggests having them in the PR in the first place.

Member

eddelbuettel commented Apr 14, 2017

Yes, I see no reason to depart from the bi-monthly schedule (apart from having a very busy May myself, but we'll cope). Any plans/concerns regarding the timeline?

I did of course skip one your packages in testing, but we've been down that road before ;-)

Also just added ChangeLog and NEWS. Alter as needed in next PR and keep in mind that Contributing.md suggests having them in the PR in the first place.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 14, 2017

Member

Doh. I guess it would help for dplyr 0.6.0?

Member

eddelbuettel commented Apr 14, 2017

Doh. I guess it would help for dplyr 0.6.0?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 14, 2017

Contributor

Don't worry, locally I have -Wextra, and I can use Rcpp from GitHub too. I was asking to assess if it's worthwhile to patch .travis.yml to enable -Wextra on Travis, but we can wait one month here.

Contributor

krlmlr commented Apr 14, 2017

Don't worry, locally I have -Wextra, and I can use Rcpp from GitHub too. I was asking to assess if it's worthwhile to patch .travis.yml to enable -Wextra on Travis, but we can wait one month here.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 14, 2017

Contributor

Thanks for editing the ChangeLog, I wasn't sure to which PR to add the change, and I sure didn't want to add three changes in three PRs... Is there a nice established way to generate change logs from GitHub commit messages?

Contributor

krlmlr commented Apr 14, 2017

Thanks for editing the ChangeLog, I wasn't sure to which PR to add the change, and I sure didn't want to add three changes in three PRs... Is there a nice established way to generate change logs from GitHub commit messages?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 14, 2017

Member

The single best support is in Emacs -- hitting C-x 4 a in any source file adds a stanza for that file.

All git2changelog converters that I have seen are deficient in the sense that you generally want to condense the entry a little (and then once more for NEWS). So I think of this more as a human-edited file. The machine readable log is indeed in git.

I just keep complaining and adding them by hand where missing. Worked for 10+ years. ;-)

Member

eddelbuettel commented Apr 14, 2017

The single best support is in Emacs -- hitting C-x 4 a in any source file adds a stanza for that file.

All git2changelog converters that I have seen are deficient in the sense that you generally want to condense the entry a little (and then once more for NEWS). So I think of this more as a human-edited file. The machine readable log is indeed in git.

I just keep complaining and adding them by hand where missing. Worked for 10+ years. ;-)

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