Skip to content

Commit 4340c97

Browse files
eddelbuetteleddelbuettel
authored
Merge branch 'master' into bugfix/module-pointer-ownership
2 parents 2798a3e + 561ddc5 commit 4340c97

17 files changed

Lines changed: 312 additions & 12 deletions

File tree

‎ChangeLog‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,20 @@
4141
* inst/tinytest/cpp/Module.cpp: Add a class deriving from another
4242
* inst/tinytest/test_module.R: Test the new checks
4343

44+
* inst/include/Rcpp/Module.h (RCPP_MODULE): Run the module body only the
45+
first time the boot function is called, since it adds to the static
46+
module and every Module() call boots the module again, which duplicated
47+
constructors and methods (#1527)
48+
* inst/include/Rcpp/module/Module.h (Module::clear): New, removes the
49+
registered classes and functions
50+
* inst/include/Rcpp/Module.h (RCPP_MODULE): Clear the module when the
51+
body throws, so that a later load runs the body against an empty module
52+
instead of appending to what the failed run registered
53+
* inst/tinytest/cpp/ModuleInitRetry.cpp: New module whose first load
54+
fails after registering a class and a function
55+
* inst/tinytest/test_module.R: Test loading a module repeatedly, and
56+
loading one again after its body threw
57+
4458
* inst/include/Rcpp/Module.h (RCPP_MODULE): Wrap the module boot
4559
function in BEGIN_RCPP / END_RCPP so that an exception thrown while the
4660
module initializes raises an R error instead of terminating the session
@@ -59,6 +73,23 @@
5973
* inst/tinytest/cpp/Module.cpp: Add a class counting its destructions
6074
* inst/tinytest/test_module.R: Test that R deletes a returned pointer
6175

76+
* inst/include/Rcpp/Module.h (internal::module_class_pointer): New
77+
per-library record of the class_<Class> registered for a C++ type
78+
* inst/include/Rcpp/Module.h (internal::make_new_object): Pass it to
79+
the new cpp_object_maker_by_class(), so that objects returned from C++
80+
are wrapped as the module class of the library that returned them
81+
rather than as another library's class of the same name (#1531)
82+
* inst/include/Rcpp/module/class.h (class_::get_instance): Record the
83+
class
84+
* R/Module.R (cpp_object_maker_by_class): New, looking the class up by
85+
its pointer and creating the object through the class's generator
86+
* R/Module.R (Module): Register classes by pointer as well
87+
* R/zzz.R: Add .classes_by_pointer
88+
* inst/tinytest/testRcppCollisionA: New test package
89+
* inst/tinytest/testRcppCollisionB: Idem
90+
* inst/tinytest/test_module_class_collision.R: Test objects returned
91+
by both
92+
6293
2026-10-23 Dirk Eddelbuettel <edd@debian.org>
6394

6495
* DESCRIPTION (Version, Date): Roll micro version and date

‎R/Module.R‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,17 @@ cpp_object_maker <- function(typeid, pointer){
161161
new( Class, .object_pointer = pointer )
162162
}
163163

164+
# called by make_new_object() with the module class that the calling library
165+
# registered for this C++ type, since other libraries' unrelated classes of
166+
# the same name share its typeid; the object is created through that class's
167+
# generator, as new() would look the reference class up by name
168+
cpp_object_maker_by_class <- function(typeid, pointer, class_pointer){
169+
Class <- .classes_by_pointer[[ externalptr_address(class_pointer) ]]
170+
if( is.null( Class ) )
171+
return( cpp_object_maker( typeid, pointer ) )
172+
Class@generator$new( .object_pointer = pointer )
173+
}
174+
164175
Module <- function( module, PACKAGE = methods::getPackageName(where), where = topenv(parent.frame()), mustStart = FALSE ) {
165176
if (inherits(module, "DLLInfo") && missing(mustStart)) mustStart <- TRUE # #nocov
166177
if (inherits(module, "Module")) {
@@ -287,6 +298,7 @@ Module <- function( module, PACKAGE = methods::getPackageName(where), where = to
287298
# rather than rebuilding it (and all of its method/field objects)
288299
CLASS@generator <- generators[[ clname ]]
289300
.classes_map[[ CLASS@typeid ]] <- storage[[ demangled_name ]] <- CLASS
301+
.classes_by_pointer[[ externalptr_address( CLASS@pointer ) ]] <- CLASS
290302

291303
# exposing enums values as CLASS.VALUE
292304
# (should really be CLASS$value but I don't know how to do it)

‎R/zzz.R‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
.dummyInstancePointer <- new.env() # just something permanent
1919

2020
.classes_map <- new.env()
21+
.classes_by_pointer <- new.env()
2122

2223
.onLoad <- function(libname, pkgname){
2324
new_dummyObject(.dummyInstancePointer) # nocov start

‎inst/include/Rcpp/Module.h‎

Lines changed: 47 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,33 @@ namespace Rcpp{
7373
} ;
7474

7575
namespace internal {
76+
77+
// the class_<Class> this shared library registered in a module, if
78+
// any. Unrelated classes of the same name in other libraries share
79+
// the typeid name, so make_new_object() passes this on for R to pick
80+
// the right module class; hidden visibility keeps one per library,
81+
// where the dynamic linker would otherwise share it between them
82+
template <typename Class>
83+
inline attribute_hidden void*& module_class_pointer(){
84+
static void* pointer = 0 ;
85+
return pointer ;
86+
}
87+
7688
template <typename Class>
7789
SEXP make_new_object( Class* ptr ){
7890
Rcpp::XPtr<Class> xp( ptr, true ) ;
79-
Function maker = Environment::Rcpp_namespace()[ "cpp_object_maker"] ;
91+
Environment rcpp = Environment::Rcpp_namespace() ;
92+
93+
// older versions of Rcpp only have cpp_object_maker()
94+
void* class_pointer = module_class_pointer<Class>() ;
95+
SEXP maker_by_class = rcpp.get( "cpp_object_maker_by_class" ) ;
96+
if( class_pointer != 0 && maker_by_class != R_NilValue ){
97+
Shield<SEXP> class_xp( R_MakeExternalPtr( class_pointer, R_NilValue, R_NilValue ) ) ;
98+
Function maker( maker_by_class ) ;
99+
return maker( typeid(Class).name(), xp, class_xp ) ;
100+
}
101+
102+
Function maker = rcpp[ "cpp_object_maker" ] ;
80103
return maker( typeid(Class).name() , xp ) ;
81104
}
82105
}
@@ -637,17 +660,29 @@ namespace Rcpp {
637660
#define RCPP_MODULE_BOOT(name) _rcpp_module_boot_##name
638661

639662
// the boot function is called from R via .Call(), so exceptions thrown by
640-
// the module body must be turned into R errors
641-
#define RCPP_MODULE(name) \
642-
void _rcpp_module_##name##_init() ; \
643-
static Rcpp::Module _rcpp_module_##name( # name ) ; \
644-
extern "C" SEXP _rcpp_module_boot_##name(){ \
645-
BEGIN_RCPP \
646-
Rcpp::internal::ModuleScope _rcpp_module_scope( & _rcpp_module_##name ) ; \
647-
_rcpp_module_##name##_init( ) ; \
648-
return Rcpp::XPtr<Rcpp::Module>( & _rcpp_module_##name , false ) ; \
649-
END_RCPP \
650-
} \
663+
// the module body must be turned into R errors; it runs every time the
664+
// module is loaded, but the body adds to the static module, so it may only
665+
// run once, and a body that throws part-way leaves nothing behind so that
666+
// the next load does not append to what it registered
667+
#define RCPP_MODULE(name) \
668+
void _rcpp_module_##name##_init() ; \
669+
static Rcpp::Module _rcpp_module_##name( # name ) ; \
670+
static bool _rcpp_module_##name##_initialized = false ; \
671+
extern "C" SEXP _rcpp_module_boot_##name(){ \
672+
BEGIN_RCPP \
673+
if( ! _rcpp_module_##name##_initialized ){ \
674+
Rcpp::internal::ModuleScope _rcpp_module_scope( & _rcpp_module_##name ) ; \
675+
try { \
676+
_rcpp_module_##name##_init( ) ; \
677+
} catch( ... ) { \
678+
_rcpp_module_##name.clear() ; \
679+
throw ; \
680+
} \
681+
_rcpp_module_##name##_initialized = true ; \
682+
} \
683+
return Rcpp::XPtr<Rcpp::Module>( & _rcpp_module_##name , false ) ; \
684+
END_RCPP \
685+
} \
651686
void _rcpp_module_##name##_init()
652687

653688
// silly little dance to suppress a 'defined but not used variable' warning

‎inst/include/Rcpp/module/Module.h‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,22 @@ namespace Rcpp {
203203
classes.insert( CLASS_PAIR( name_ , cptr ) ) ;
204204
}
205205

206+
// drops everything the module body registered; used when the body
207+
// throws, so that running it again starts from an empty module (the
208+
// C callables registered by Add() cannot be removed, but running the
209+
// body again simply registers them again)
210+
inline void clear(){
211+
for( MAP::iterator it = functions.begin(); it != functions.end(); ++it ){
212+
delete it->second ;
213+
}
214+
functions.clear() ;
215+
216+
for( CLASS_MAP::iterator it = classes.begin(); it != classes.end(); ++it ){
217+
delete it->second ;
218+
}
219+
classes.clear() ;
220+
}
221+
206222
inline bool has_function( const std::string& m){ // #nocov start
207223
return functions.find(m) != functions.end() ;
208224
}

‎inst/include/Rcpp/module/class.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@
8989
RCPP_DEBUG_MODULE_2( "class_<%s>::get_instance(). [freshly created] class_pointer = class_pointer\n", DEMANGLE(Class), class_pointer ) ;
9090

9191
}
92+
93+
// lets objects returned from C++ find this module class
94+
internal::module_class_pointer<Class>() = static_cast<class_Base*>( class_pointer ) ;
95+
9296
return class_pointer ;
9397
}
9498

‎inst/tinytest/cpp/ModuleInitRetry.cpp‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; indent-tabs-mode: nil; -*-
2+
//
3+
// ModuleInitRetry.cpp: Rcpp R/C++ interface class library -- module whose
4+
// initialization fails the first time only
5+
//
6+
// Copyright (C) 2026 Kevin Ushey
7+
//
8+
// This file is part of Rcpp.
9+
//
10+
// Rcpp is free software: you can redistribute it and/or modify it
11+
// under the terms of the GNU General Public License as published by
12+
// the Free Software Foundation, either version 2 of the License, or
13+
// (at your option) any later version.
14+
//
15+
// Rcpp is distributed in the hope that it will be useful, but
16+
// WITHOUT ANY WARRANTY; without even the implied warranty of
17+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
18+
// GNU General Public License for more details.
19+
//
20+
// You should have received a copy of the GNU General Public License
21+
// along with Rcpp. If not, see <http://www.gnu.org/licenses/>.
22+
23+
#include <Rcpp.h>
24+
25+
using namespace Rcpp;
26+
27+
class ModuleInitRetry {
28+
public:
29+
ModuleInitRetry() {}
30+
int value() { return 42; }
31+
};
32+
33+
int retry_add_one(int x) {
34+
return x + 1;
35+
}
36+
37+
static int boot_count = 0;
38+
39+
// registers a class and a function, then throws on the first boot only, so
40+
// the second boot must not find what the first one registered
41+
RCPP_MODULE(moduleInitRetry) {
42+
class_<ModuleInitRetry>("ModuleInitRetry")
43+
.constructor()
44+
.method("value", &ModuleInitRetry::value)
45+
;
46+
function("retry_add_one", &retry_add_one);
47+
48+
if (++boot_count == 1)
49+
throw std::runtime_error("first boot fails");
50+
}

‎inst/tinytest/testRcppCollisionA/DESCRIPTION‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
Package: testRcppCollisionA
2+
Type: Package
3+
Title: Module Class Name Collision Test A
4+
Version: 0.1
5+
Author: Kevin Ushey
6+
Maintainer: Kevin Ushey <kevinushey@gmail.com>
7+
Description: Exposes a module class whose name is also used by an unrelated
8+
class in package testRcppCollisionB, for the Rcpp tests.
9+
License: GPL (>= 2)
10+
Imports: methods, Rcpp
11+
LinkingTo: Rcpp

‎inst/tinytest/testRcppCollisionA/NAMESPACE‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
useDynLib(testRcppCollisionA)
2+
import(methods, Rcpp)
3+
export(Collision, make_collision)

‎inst/tinytest/testRcppCollisionA/R/zzz.R‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
loadModule("collision_module", TRUE)

0 commit comments

Comments
 (0)