maintain existing interface and offer new one

[eddelbuettel]

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 in ChangeLog

[kevinushey]

LGTM!

[Enchufa2]

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 to Rcpp_precious_preserve and Rcpp_precious_remove here:

Rcpp/inst/include/Rcpp/traits/named_object.h

Lines 40 to 59 in 2c2eb89

	template <> class named_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 been Rcpp_PreserveObject and Rcpp_ReleaseObject instead, and thus Rcpp_PreciousPreserve and Rcpp_PreciousRelease in this PR under the new API.

[eddelbuettel]

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]

Codecov Report

Merging #1135 (0dedb16) into master (4ed72aa) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1135   +/-   ##
=======================================
  Coverage   97.43%   97.43%           
=======================================
  Files          64       64           
  Lines        2764     2764           
=======================================
  Hits         2693     2693           
  Misses         71       71           

Impacted Files	Coverage Δ
inst/include/Rcpp/traits/named_object.h	100.00% <ø> (ø)
inst/include/Rcpp/storage/PreserveStorage.h	100.00% <100.00%> (ø)
inst/include/RcppCommon.h	100.00% <100.00%> (ø)

[eddelbuettel]

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.

[jeroen]

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.

[eddelbuettel]

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.