Skip to content

document ownership of objects returned to r from modules - #1534

Open
kevinushey wants to merge 1 commit into
masterfrom
bugfix/module-pointer-ownership
Open

kevinushey wants to merge 1 commit into
masterfrom
bugfix/module-pointer-ownership

Conversation

@kevinushey

Copy link
Copy Markdown
Contributor

Fixes #1533.

A raw pointer (T*) returned from a module function or method is wrapped in a new R object that owns it (make_new_object() creates XPtr<T>(ptr, true)), so the object is deleted when that R object is collected. That is what factory-style code returning new T(...) relies on, so this PR doesn't change it. But it wasn't documented, and returning this (to chain calls), a pointer to a member or container element, or the same pointer twice deletes the object more than once. See #1533 for the reprex.

Changes:

  • vignettes/rmd/Rcpp-modules.Rmd: new subsection "Returning objects of exposed classes", after "Extending Rcpp::as and Rcpp::wrap". It explains that objects returned by value or by reference are copied and owned by R, while a returned pointer is owned and eventually deleted by R, so it may only be returned when nothing else owns the object. It lists the patterns that lead to double deletes, with a short example. Only the Rmd source changes; the PDF can be regenerated with the other vignettes at release time.
  • inst/tinytest/cpp/Module.cpp, inst/tinytest/test_module.R: a ModuleOwned class that counts its destructions, and a test that a pointer returned from ModuleOwned_create() is deleted once the R object wrapping it is garbage collected. This pins down the documented behavior. It passes on master too, since nothing changes in the code.

Verification:

  • Checked the documented copy semantics: modifying the object returned by a method returning T&, or a free function returning T&, leaves the original untouched.
  • test_module.R (47/47) passes with RunAllRcppTests=yes (R 4.6.1, macOS).

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests (test_module.R run locally; documentation-only otherwise)
  • Preferably, new tests were added which fail without the change (not applicable: the new test pins existing behavior that is now documented)
  • Document the changes by file in ChangeLog

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.

returning a pointer from a module method hands ownership to r, so returning this double-frees

1 participant