Warning from -Wdelete-non-virtual-dtor when using a .finalizer function about rcpp

[ctoney]

From https://stackoverflow.com/questions/59384221/proper-way-to-return-a-pointer-to-a-new-object-from-an-rcpp-function

Rcpp Modules register a default finalizer that calls the object's destructor. You can also add a custom finalizer if needed.

Sorry, I didn't catch that before. My rationale, originally, was that a custom finalizer would run upon gc() but the class destructor may be less predictable. If the above is correct, and the destructor does the necessary clean-up, then a custom finalizer may not be needed. Makes sense.

It seems to work fine either way for class VSIFile, and the destructor is called during garbage collection when not using .finalizer. I'll omit the custom finalizer in this case.

[ctoney]

Intended to circle back and let you know it is on CRAN but got distracted by a couple of things. My issue is resolved, but by not using the custom finalizer. The object's destructor is called at gc(), since Rcpp Modules registers a default finalizer as noted above (but I missed that previously). That meets my need and the cleanup code is very simple. The compiler warning appears though, if a custom .finalizer is defined on the exposed class, in case that's of interest. Thanks again.

[eddelbuettel]

Good question. Modules has not seen a lot of love those last few years.

If your repo with added finalizer public? Not sure we have (m)any other examples of finalizers in use (though I have not looked yet).

[kevinushey]

I think you're right; the class has virtual methods but no virtual destructor:

Rcpp/inst/include/Rcpp/Module.h

Lines 299 to 304 in 03fd09a

	template <typename Class>
	class CppFinalizer{
	public:
	CppFinalizer(){} ;
	virtual void run(Class* ){} ;
	} ;

We should probably add it, or just simplify and remove the virtual inheritance used here (since FunctionFinalizer is the only thing that inherits from CppFinalizer)

[eddelbuettel]

What do you think about e.g. rule of four adding the missing ones as defaults? (I am spitballing here...)

[ctoney]

Thanks for the quick response.

If your repo with added finalizer public?

It's currently in a branch of my fork here, but will be merged sometime soon.
https://github.com/ctoney/gdalraster/blob/vsifile/src/vsifile.h
https://github.com/ctoney/gdalraster/blob/vsifile/src/vsifile.cpp

I have a couple of other exposed classes that I would add finalizers for, but didn't have it working in the past so spending a little more time to check it out now.

Modules has not seen a lot of love those last few years.

FWIW, I really like modules and RCPP_EXPOSED_CLASS. Super useful and nice to work with. Thanks!

[eddelbuettel]

Do you want to give modifying the set of defaults and see where it leads you?

[ctoney]

update to the above, the class with finalizer has been merged in main:
https://github.com/USDAForestService/gdalraster/blob/main/src/vsifile.h
https://github.com/USDAForestService/gdalraster/blob/main/src/vsifile.cpp

[eddelbuettel]

That's actually a pretty nice example for modules use!

[ctoney]

It's a nice API to be able to wrap and use from R. Class VSIFile is in pretty good shape now I think, and has a decent set of tests for /vsimem/ at least. There is also a set of Rcpp exported functions for file system operations through GDAL VSI. Among other things, these fully abstract file system operations for cloud storage services.
https://usdaforestservice.github.io/gdalraster/reference/index.html#virtual-file-systems

And as side note, VSIFile was initial use of RcppInt64 for this package. It made that part easy too. I was glad to add it and start using for int64 in a few other places as well.

[eddelbuettel]

Hey @ctoney just circling back and as you (per CRANberries) have a new version on CRAN: are you all good here? What if any action items do we have left?