Rcpp
Rcpp copied to clipboard
RcppCore
Warning from -Wdelete-non-virtual-dtor when using a .finalizer function
Using modules with an RCPP_EXPOSED_CLASS that has a .finalizer function, a compiler warning is emitted:
vsifile.cpp:367:15: required from here
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/module/class.h:466:42: warning: deleting object of polymorphic class type ‘Rcpp::CppFinalizer<VSIFile>’ which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor]
466 | if( ptr->finalizer_pointer ) delete ptr->finalizer_pointer ;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Is this just because Rcpp::CppFinalizer does not have virtual destructor? Or more likely involves something on my end?
Thanks.
g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
> sessionInfo()
R version 4.4.0 (2024-04-24)
Platform: x86_64-pc-linux-gnu
Running under: Ubuntu 22.04.4 LTS
Matrix products: default
BLAS: /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.10.0
LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.10.0
locale:
[1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C
[3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8
[5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=en_US.UTF-8 LC_NAME=C
[9] LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C
time zone: America/Denver
tzcode source: system (glibc)
attached base packages:
[1] stats graphics grDevices utils datasets methods base
other attached packages:
[1] gdalraster_1.10.9180
loaded via a namespace (and not attached):
[1] utf8_1.2.4 xml2_1.3.6 stringi_1.8.4 digest_0.6.35
[5] magrittr_2.0.3 pkgload_1.3.4 fastmap_1.2.0 rprojroot_2.0.4
[9] processx_3.8.4 RcppInt64_0.0.5 pkgbuild_1.4.4 sessioninfo_1.2.2
[13] ps_1.7.6 urlchecker_1.0.1 promises_1.3.0 purrr_1.0.2
[17] fansi_1.0.6 codetools_0.2-20 cli_3.6.2 shiny_1.8.1.1
[21] rlang_1.1.3 commonmark_1.9.1 bit64_4.0.5 ellipsis_0.3.2
[25] remotes_2.5.0 withr_3.0.0 cachem_1.1.0 devtools_2.4.5
[29] tools_4.4.0 memoise_2.0.1 httpuv_1.6.15 vctrs_0.6.5
[33] R6_2.5.1 mime_0.12 lifecycle_1.0.4 stringr_1.5.1
[37] bit_4.0.5 fs_1.6.4 htmlwidgets_1.6.4 usethis_2.2.3
[41] miniUI_0.1.1.1 pkgconfig_2.0.3 desc_1.4.3 callr_3.7.6
[45] pillar_1.9.0 later_1.3.2 glue_1.7.0 profvis_0.3.8
[49] Rcpp_1.0.12 xfun_0.44 tibble_3.2.1 rstudioapi_0.16.0
[53] knitr_1.46 xtable_1.8-4 htmltools_0.5.8.1 compiler_4.4.0
[57] roxygen2_7.3.1
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).
I think you're right; the class has virtual methods but no virtual destructor:
https://github.com/RcppCore/Rcpp/blob/03fd09a1bd0b4dce5be262dd95f779d9a855e097/inst/include/Rcpp/Module.h#L299-L304
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)
What do you think about e.g. rule of four adding the missing ones as defaults? (I am spitballing here...)
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!
Do you want to give modifying the set of defaults and see where it leads you?
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
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.
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.
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?
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.