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

Rewrite no_init() and other functions to reduce false positives #893

Merged
merged 6 commits into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@krlmlr
Contributor

krlmlr commented Aug 18, 2018

Reference: #892.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 18, 2018

Codecov Report

Merging #893 into master will increase coverage by <.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
+ Coverage   90.14%   90.15%   +<.01%     
==========================================
  Files          71       71              
  Lines        3279     3282       +3     
==========================================
+ Hits         2956     2959       +3     
  Misses        323      323
Impacted Files Coverage Δ
inst/include/Rcpp/exceptions.h 7.69% <ø> (ø) ⬆️
inst/include/Rcpp/protection/Shelter.h 0% <0%> (ø) ⬆️
inst/include/Rcpp/api/meat/Rcpp_eval.h 66.66% <100%> (ø) ⬆️
inst/include/Rcpp/protection/Shield.h 100% <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 522e66d...b735942. Read the comment docs.

codecov-io commented Aug 18, 2018

Codecov Report

Merging #893 into master will increase coverage by <.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
+ Coverage   90.14%   90.15%   +<.01%     
==========================================
  Files          71       71              
  Lines        3279     3282       +3     
==========================================
+ Hits         2956     2959       +3     
  Misses        323      323
Impacted Files Coverage Δ
inst/include/Rcpp/exceptions.h 7.69% <ø> (ø) ⬆️
inst/include/Rcpp/protection/Shelter.h 0% <0%> (ø) ⬆️
inst/include/Rcpp/api/meat/Rcpp_eval.h 66.66% <100%> (ø) ⬆️
inst/include/Rcpp/protection/Shield.h 100% <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 522e66d...b735942. Read the comment docs.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Aug 18, 2018

Contributor

My understanding is that rchk was primarily designed to diagnose C rather than C++ code. I would prefer not making these kinds of changes just to appease the tool unless we really have a good reason.

Contributor

kevinushey commented Aug 18, 2018

My understanding is that rchk was primarily designed to diagnose C rather than C++ code. I would prefer not making these kinds of changes just to appease the tool unless we really have a good reason.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Aug 19, 2018

Contributor

The proposed changes reduce false positives reported by rchk not only in Rcpp, but in packages that merely use Rcpp::no_init() (directly or indirectly). CRAN uses rchk, fixing these issues will help them. (We've been asked about these when submitting dplyr 0.7.6, we could resolve many but not all on the client side by using the second syntax variant in #892.) I added comments to explain the changes.

The rchk documentation recommends rewriting code to reduce the number of false positives too.

Code that relies entirely on Rcpp's vector classes doesn't benefit much from rchk. On the other hand, ideally such code shouldn't produce as few false positives as possible, ideally none.

Contributor

krlmlr commented Aug 19, 2018

The proposed changes reduce false positives reported by rchk not only in Rcpp, but in packages that merely use Rcpp::no_init() (directly or indirectly). CRAN uses rchk, fixing these issues will help them. (We've been asked about these when submitting dplyr 0.7.6, we could resolve many but not all on the client side by using the second syntax variant in #892.) I added comments to explain the changes.

The rchk documentation recommends rewriting code to reduce the number of false positives too.

Code that relies entirely on Rcpp's vector classes doesn't benefit much from rchk. On the other hand, ideally such code shouldn't produce as few false positives as possible, ideally none.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 19, 2018

Member

A quick check at GitHub searching 'user:cran no_init' shows that there are about a handful of packages using no_init() so it may be worth it.

A reverse-depends run seems to suggest no new issues (package 'vapour' came up failing but I also see the same issue for macOS at its CRAN checks so this may not be us).

What about no_init.h though and Kevin's comment? Looking at the file I see two cases for vector and matrix. You addressed only one. Should probably be both. But then again, why not keep Rf_allocVector() and Rf_allocMatrix() ?

Member

eddelbuettel commented Aug 19, 2018

A quick check at GitHub searching 'user:cran no_init' shows that there are about a handful of packages using no_init() so it may be worth it.

A reverse-depends run seems to suggest no new issues (package 'vapour' came up failing but I also see the same issue for macOS at its CRAN checks so this may not be us).

What about no_init.h though and Kevin's comment? Looking at the file I see two cases for vector and matrix. You addressed only one. Should probably be both. But then again, why not keep Rf_allocVector() and Rf_allocMatrix() ?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Aug 19, 2018

Contributor

Thanks. I'll address the matrix case and also try to simplify the code more.

I still think we need to return an object with protection, if we keep the StoragePolicy template argument we might end up with unprotected objects again.

I don't understand "keep Rf_allocVector()": the code still uses Rf_allocVector() but just protects it explicitly.

Contributor

krlmlr commented Aug 19, 2018

Thanks. I'll address the matrix case and also try to simplify the code more.

I still think we need to return an object with protection, if we keep the StoragePolicy template argument we might end up with unprotected objects again.

I don't understand "keep Rf_allocVector()": the code still uses Rf_allocVector() but just protects it explicitly.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Aug 20, 2018

Contributor

We've been asked about these when submitting dplyr 0.7.6

Since CRAN is enforcing these checks I agree we should do what we can to reduce false positives.

Contributor

kevinushey commented Aug 20, 2018

We've been asked about these when submitting dplyr 0.7.6

Since CRAN is enforcing these checks I agree we should do what we can to reduce false positives.

@@ -243,7 +243,7 @@ namespace internal {
inline bool is_Rcpp_eval_call(SEXP expr) {
SEXP sys_calls_symbol = Rf_install("sys.calls");
SEXP identity_symbol = Rf_install("identity");
SEXP identity_fun = Rf_findFun(identity_symbol, R_BaseEnv);
Shield<SEXP> identity_fun(Rf_findFun(identity_symbol, R_BaseEnv));
SEXP tryCatch_symbol = Rf_install("tryCatch");

This comment has been minimized.

@kevinushey

kevinushey Aug 20, 2018

Contributor

Does rchk warn about the unprotected use of 'install' here?

@kevinushey

kevinushey Aug 20, 2018

Contributor

Does rchk warn about the unprotected use of 'install' here?

This comment has been minimized.

@krlmlr

krlmlr Aug 21, 2018

Contributor

This comment has been minimized.

@eddelbuettel

eddelbuettel Aug 21, 2018

Member

So should the PR cover that too then?

@eddelbuettel

eddelbuettel Aug 21, 2018

Member

So should the PR cover that too then?

This comment has been minimized.

@krlmlr

krlmlr Aug 21, 2018

Contributor

I was confused. No, rchk knows that the Rf_install() result is protected already, but Rf_install() could be an allocation (in theory) which could clean up the result of Rf_findFun().

@krlmlr

krlmlr Aug 21, 2018

Contributor

I was confused. No, rchk knows that the Rf_install() result is protected already, but Rf_install() could be an allocation (in theory) which could clean up the result of Rf_findFun().

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 26, 2018

Member

Thanks for coming back to this -- looks pretty good now (though it is early here and the coffee may not have settled ... ;-)

One thing still missing is the Matrix case on no_init.h. Could you add that?

Member

eddelbuettel commented Aug 26, 2018

Thanks for coming back to this -- looks pretty good now (though it is early here and the coffee may not have settled ... ;-)

One thing still missing is the Matrix case on no_init.h. Could you add that?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Aug 26, 2018

Contributor

Thanks for the heads-up, I forgot. (Need to wait for the checks.)

Contributor

krlmlr commented Aug 26, 2018

Thanks for the heads-up, I forgot. (Need to wait for the checks.)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 26, 2018

Member

Err, don't you test locally before sending a commit ?

Member

eddelbuettel commented Aug 26, 2018

Err, don't you test locally before sending a commit ?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Aug 26, 2018

Contributor

Guilty as charged.

Contributor

krlmlr commented Aug 26, 2018

Guilty as charged.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 26, 2018

Member

I may not be entirely innocent of the practice either but I hate the red outcomes with such a vengeance that I try avoid this. Nice use of git commit --amend though :)

Member

eddelbuettel commented Aug 26, 2018

I may not be entirely innocent of the practice either but I hate the red outcomes with such a vengeance that I try avoid this. Nice use of git commit --amend though :)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 26, 2018

Member

I will set up another reverse-depends check. Not expecting any trouble. Should be ready to merge.

@kevinushey @jjallaire @thirdwing @nathan-russell and anybody else: Comments? Concerns?

Member

eddelbuettel commented Aug 26, 2018

I will set up another reverse-depends check. Not expecting any trouble. Should be ready to merge.

@kevinushey @jjallaire @thirdwing @nathan-russell and anybody else: Comments? Concerns?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 26, 2018

Member

I canceled and restarted that one, and (of course) just after I canceled it completed. It had exit-ed fine with value of zero but was hanging there...

Member

eddelbuettel commented Aug 26, 2018

I canceled and restarted that one, and (of course) just after I canceled it completed. It had exit-ed fine with value of zero but was hanging there...

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Aug 26, 2018

Contributor

Thanks. Looks super-green now.

Contributor

krlmlr commented Aug 26, 2018

Thanks. Looks super-green now.

@krlmlr krlmlr changed the title from WIP: rewrite no_init() and other functions to reduce false positives to Rewrite no_init() and other functions to reduce false positives Aug 26, 2018

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 26, 2018

Member

No new issue in level-one reverse depends check, results at RcppCore/rcpp-logs@1d9d5fa

Member

eddelbuettel commented Aug 26, 2018

No new issue in level-one reverse depends check, results at RcppCore/rcpp-logs@1d9d5fa

@eddelbuettel eddelbuettel merged commit 4813640 into RcppCore:master Aug 27, 2018

1 check passed

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

@krlmlr krlmlr deleted the krlmlr:f-no-init-protect branch Sep 15, 2018

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