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

Add argument to XPtr template to run finalizer on exit (fixes #655) #656

Merged
merged 2 commits into from
Mar 16, 2017
Merged

Add argument to XPtr template to run finalizer on exit (fixes #655) #656

merged 2 commits into from
Mar 16, 2017

Conversation

jeroen
Copy link
Contributor

@jeroen jeroen commented Mar 15, 2017

See #655. This adds an additional parameter to XPtr which specifies if the finalizer should run on exit in the same spirit as the underlying R_RegisterCFinalizerEx.

I confirmed that this works. Default is still FALSE so the current behavior will be unchanged.

@codecov-io
Copy link

codecov-io commented Mar 15, 2017

Codecov Report

Merging #656 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master     #656   +/-   ##
=======================================
  Coverage   92.91%   92.91%           
=======================================
  Files          65       65           
  Lines        3303     3303           
=======================================
  Hits         3069     3069           
  Misses        234      234
Impacted Files Coverage Δ
inst/include/Rcpp/XPtr.h 62.16% <0%> (ø)

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 0566d7c...e3799de. Read the comment docs.

Copy link
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

@jeroen
Copy link
Contributor Author

jeroen commented Mar 15, 2017

OK perfect, I'll add a NEWS item and bump the version.

@eddelbuettel
Copy link
Member

Great, and a changelog entry, please.

Are the data structures in your client packages C or C++? I am just wondering if you could also get this for free for C++ destructor semantics on your side . Just checking ...

inst/NEWS.Rd Outdated
@@ -7,6 +7,8 @@
\itemize{
\item Changes in Rcpp API:
\itemize{
\item XPtr gains a parameter \code{finalizeoOnExit} to enable running the
Copy link
Member

Choose a reason for hiding this comment

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

Spelling of parameter here (finalizeoOnExit should be finalizeOnExit)

Copy link
Member

Choose a reason for hiding this comment

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

May also move the entry to the bottom. They are mostly chronological. In any even I can clean up both when finalizing the release "real soon now".

@jjallaire
Copy link
Member

@eddelbuettel I think the default finalizer will delete the C++ object thereby calling the destructor (so without this change there is actually no way to guarantee that the destructor is called prior to the process exiting)

@jeroen FWIW in some cases I've found it better to not let C++ destructors get called at process exit because the runtime context of an exiting process can be so stripped down (or non-deterministic across multiple C++ objects getting destroyed) that the destructor either crashes or hangs. i.e. sometimes it's better to just let the OS clean up than to try to explicitly do so within the process. Obviously this doesn't work in all cases, but I know that many things like mutexes, sockets, file handles, memory, etc. are freed automatically by the OS at process exit.

@eddelbuettel
Copy link
Member

Leaving the default at false is definitely the way to go.

@jeroen
Copy link
Contributor Author

jeroen commented Mar 15, 2017

OK fixed the typo and moved the \item{} to the bottom.

@eddelbuettel
Copy link
Member

I launched a full reverse-depends check just in case, but we won't know til tomorrow how it does.

@eddelbuettel
Copy link
Member

Rev deps were all fine, results in RcppCore/rcpp-logs@8103eff

@eddelbuettel eddelbuettel merged commit 6cf1301 into RcppCore:master Mar 16, 2017
@jeroen jeroen deleted the xptr-onexit branch March 16, 2017 16:11
@jeroen
Copy link
Contributor Author

jeroen commented Mar 16, 2017

Rev deps were all fine, results in RcppCore/rcpp-logs@8103eff

Let's send it to CRAN then :D

@eddelbuettel
Copy link
Member

Yes, which is why I ran the two previous reverse-depends. Probably by Friday.

Given the long tail of these reverse dependencies, Rcpp is a littler slower to make it from Incoming but you should be able to rely on it come next week.

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