Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

maintain existing interface and offer new one#1135

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
eddelbuettel merged 2 commits intomasterfromfeature/preserve_interface
Jan 20, 2021

Conversation

@eddelbuettel
Copy link
Member

@eddelbuetteleddelbuettel commentedJan 19, 2021•
edited
Loading

This does a partial revert of#1133 in the sense that we should not have altered the existing top-level API (my bad for sleeping on that, and thanks to@kevinushey for the wake-up on it). The use of the preserve/release pair is however not all that widespread so this PR switches to using the new token / precious list via two new functions.

Eventually, as hinted by@kevinushey in this#1133 (comment) we may want to rework this.

A simpler solution may just be to gradually deprecate and remove the three old functions -- as the runs this weekend showed they do not appear to be used. So we could keep them and demote to another internal header that gets pulled if a new#define is set and over time we flip is default value but let other users set it if they need it.

/cc@Enchufa2 whom I can't seem to tag under reviewers

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Prefereably, new tests were added which fail without the change
  • Document the changes by file inChangeLog

Copy link
Contributor

@kevinusheykevinushey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM!

eddelbuettel reacted with thumbs up emoji
@Enchufa2
Copy link
Member

Thanks@kevinushey for raising this issue. Completely agree. This LGTM too. Just a small detail: on second pass, I believe that the calls I made toRcpp_precious_preserve andRcpp_precious_remove here:

template<>classnamed_object<SEXP> {
public:// #nocov start
named_object(const std::string& name_,const SEXP& o_):
name(name_), object(o_), token(R_NilValue) {
token =Rcpp_precious_preserve(object);
}
named_object(const named_object<SEXP>& other ) :
name(other.name), object(other.object), token(other.token) {
token =Rcpp_precious_preserve(object);
}
~named_object() {
Rcpp_precious_remove(token);
}// #nocov end
const std::string& name;
SEXP object;
private:
SEXP token;
};

should have beenRcpp_PreserveObject andRcpp_ReleaseObject instead, and thusRcpp_PreciousPreserve andRcpp_PreciousRelease in this PR under the new API.

eddelbuettel reacted with thumbs up emoji

@eddelbuettel
Copy link
MemberAuthor

It's just two inlined one-liners but I agree that it nicer to see the top-level API function used -- so committed (and tested locally and pushed).

@codecov-io
Copy link

codecov-io commentedJan 19, 2021•
edited
Loading

Codecov Report

Merging#1135 (0dedb16) intomaster (4ed72aa) willnot change coverage.
The diff coverage is100.00%.

@@           Coverage Diff           @@##           master    #1135   +/-   ##=======================================  Coverage   97.43%   97.43%           =======================================  Files          64       64             Lines        2764     2764           =======================================  Hits         2693     2693             Misses         71       71
Impacted FilesCoverage Δ
inst/include/Rcpp/traits/named_object.h100.00% <ø> (ø)
inst/include/Rcpp/storage/PreserveStorage.h100.00% <100.00%> (ø)
inst/include/RcppCommon.h100.00% <100.00%> (ø)

@eddelbuettel
Copy link
MemberAuthor

Thanks for the quick feedback on this, and for highlighting it in the first place. Will fold it now, and 1.0.6.3 is where we're at now.

Enchufa2 reacted with hooray emoji

@eddelbuetteleddelbuettel merged commit642f1bd intomasterJan 20, 2021
@eddelbuetteleddelbuettel deleted the feature/preserve_interface branchJanuary 20, 2021 03:02
@jeroen
Copy link
Contributor

FYI, this may have an (unintended?) side effect that packages built against new versions of Rcpp cannot be loaded with Rcpp 1.0.6. Hence, once this is released to CRAN, and CRAN rebuilds binaries against Rcpp 1.0.7, users that still have Rcpp 1.0.6 will get loading errors.

Screen Shot 2021-06-25 at 10 25 59 PM

@eddelbuettel
Copy link
MemberAuthor

That sounds unfortunate but this#1135 fixes an earlier bug that needed fixing (#1133) in the actual Rcpp 1.0.6 release. And users should be also fine by upgrading to Rcpp 1.0.7.

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

Reviewers

@kevinusheykevinusheykevinushey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@eddelbuettel@Enchufa2@codecov-io@jeroen@kevinushey
[8]ページ先頭
©2009-2025 Movatter.jp