Skip to content

Commit 2c56e1a

Browse files
authored
Merge pull request #3 from pachadotdev/issue452
Issue452
2 parents 36ef9b7 + 224b9af commit 2c56e1a

File tree

4 files changed

+132
-78
lines changed

4 files changed

+132
-78
lines changed

R/source.R

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,13 @@
1818
#' uses 'CXX11' if unset.
1919
#' @param dir The directory to store the generated source files. `tempfile()` is
2020
#' used by default. The directory will be removed if `clean` is `TRUE`.
21+
#' @param local Passed to [dyn.load()]. If `TRUE` (the default) the shared
22+
#' library is loaded with local symbols; if `FALSE` symbols are made global
23+
#' (equivalent to `dyn.load(..., local = FALSE)`), which can be required when
24+
#' other shared objects need to see RTTI/vtable symbols from this library.
25+
#' @note See the unit test that demonstrates this usage at
26+
#' \code{tests/testthat/test-source-local.R} (shows how `local = FALSE` exports
27+
#' the necessary symbols so separate shared objects can link against them).
2128
#' @return For [cpp_source()] and `[cpp_function()]` the results of
2229
#' [dyn.load()] (invisibly). For `[cpp_eval()]` the results of the evaluated
2330
#' expression.
@@ -65,7 +72,7 @@
6572
#' }
6673
#'
6774
#' @export
68-
cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, quiet = TRUE, cxx_std = Sys.getenv("CXX_STD", "CXX11"), dir = tempfile()) {
75+
cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, quiet = TRUE, cxx_std = Sys.getenv("CXX_STD", "CXX11"), dir = tempfile(), local = TRUE) {
6976
stop_unless_installed(c("brio", "callr", "cli", "decor", "desc", "glue", "tibble", "vctrs"))
7077
if (!missing(file) && !file.exists(file)) {
7178
stop("Can't find `file` at this path:\n", file, "\n", call. = FALSE)
@@ -145,7 +152,7 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu
145152
brio::write_lines(r_functions, r_path)
146153
source(r_path, local = env)
147154

148-
dyn.load(shared_lib, local = TRUE, now = TRUE)
155+
dyn.load(shared_lib, local = local, now = TRUE)
149156
}
150157

151158
the <- new.env(parent = emptyenv())
@@ -183,7 +190,7 @@ generate_makevars <- function(includes, cxx_std) {
183190

184191
#' @rdname cpp_source
185192
#' @export
186-
cpp_function <- function(code, env = parent.frame(), clean = TRUE, quiet = TRUE, cxx_std = Sys.getenv("CXX_STD", "CXX11")) {
193+
cpp_function <- function(code, env = parent.frame(), clean = TRUE, quiet = TRUE, cxx_std = Sys.getenv("CXX_STD", "CXX11"), local = TRUE) {
187194
cpp_source(code = paste(c('#include "cpp11.hpp"',
188195
"using namespace ::cpp11;",
189196
"namespace writable = ::cpp11::writable;",
@@ -193,15 +200,16 @@ cpp_function <- function(code, env = parent.frame(), clean = TRUE, quiet = TRUE,
193200
env = env,
194201
clean = clean,
195202
quiet = quiet,
196-
cxx_std = cxx_std
203+
cxx_std = cxx_std,
204+
local = local
197205
)
198206
}
199207

200208
utils::globalVariables("f")
201209

202210
#' @rdname cpp_source
203211
#' @export
204-
cpp_eval <- function(code, env = parent.frame(), clean = TRUE, quiet = TRUE, cxx_std = Sys.getenv("CXX_STD", "CXX11")) {
212+
cpp_eval <- function(code, env = parent.frame(), clean = TRUE, quiet = TRUE, cxx_std = Sys.getenv("CXX_STD", "CXX11"), local = TRUE) {
205213
cpp_source(code = paste(c('#include "cpp11.hpp"',
206214
"using namespace ::cpp11;",
207215
"namespace writable = ::cpp11::writable;",
@@ -214,7 +222,8 @@ cpp_eval <- function(code, env = parent.frame(), clean = TRUE, quiet = TRUE, cxx
214222
env = env,
215223
clean = clean,
216224
quiet = quiet,
217-
cxx_std = cxx_std
225+
cxx_std = cxx_std,
226+
local = local
218227
)
219228
f()
220229
}

cpp11test/src/test-integers.cpp

Lines changed: 0 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -138,23 +138,6 @@ context("integers-C++") {
138138
expect_true(x[1] == 3);
139139
expect_true(x[2] == 4);
140140
}
141-
test_that("integers.value()") {
142-
cpp11::writable::integers x;
143-
x.push_back(1);
144-
x.push_back(2);
145-
x.push_back(3);
146-
147-
// Test that value() returns the same as operator[] but as T directly
148-
expect_true(x.value(0) == 1);
149-
expect_true(x.value(1) == 2);
150-
expect_true(x.value(2) == 3);
151-
152-
// Test that value() works with C-style formatting (this was the original issue in
153-
// #453)
154-
expect_true(x.value(0) == x[0]);
155-
expect_true(x.value(1) == x[1]);
156-
expect_true(x.value(2) == x[2]);
157-
}
158141

159142
test_that("writable::integers(SEXP)") {
160143
SEXP x = PROTECT(Rf_allocVector(INTSXP, 5));
@@ -295,56 +278,4 @@ context("integers-C++") {
295278
int y = NA_INTEGER;
296279
expect_true(cpp11::is_na(y));
297280
}
298-
299-
test_that("proxy issue demonstration") {
300-
// This test demonstrates the proxy issue and shows that all solutions work
301-
cpp11::writable::integers x;
302-
for (int i = 0; i < 3; i++) {
303-
x.push_back(i * 10);
304-
}
305-
306-
// Test that value() method works correctly
307-
expect_true(x.value(0) == 0);
308-
expect_true(x.value(1) == 10);
309-
expect_true(x.value(2) == 20);
310-
311-
// Test that explicit cast works
312-
expect_true((int)x[0] == 0);
313-
expect_true((int)x[1] == 10);
314-
expect_true((int)x[2] == 20);
315-
316-
// Test that auto assignment works (triggers implicit conversion)
317-
int val0 = x[0];
318-
int val1 = x[1];
319-
int val2 = x[2];
320-
expect_true(val0 == 0);
321-
expect_true(val1 == 10);
322-
expect_true(val2 == 20);
323-
324-
// Test that value() and operator[] return equivalent results
325-
expect_true(x.value(0) == (int)x[0]);
326-
expect_true(x.value(1) == (int)x[1]);
327-
expect_true(x.value(2) == (int)x[2]);
328-
}
329-
}
330-
331-
// [[cpp11::register]]
332-
// Demo function to show the three ways to handle the proxy issue
333-
// To use this function:
334-
// 1. Run cpp11::cpp_register() to regenerate R bindings
335-
// 2. Rebuild and reinstall the package
336-
// 3. Call test_proxy_issue_demo() from R
337-
void test_proxy_issue_demo() {
338-
cpp11::writable::integers x;
339-
for (int i = 0; i < 5; i++) {
340-
x.push_back(i);
341-
342-
// These all work correctly:
343-
Rprintf("Method 1 - cast: x[%d] = %d\n", i, (int)x[i]);
344-
Rprintf("Method 2 - value(): x[%d] = %d\n", i, x.value(i));
345-
346-
// This also works (auto triggers implicit conversion):
347-
int val = x[i];
348-
Rprintf("Method 3 - auto: x[%d] = %d\n", i, val);
349-
}
350281
}

man/cpp_source.Rd

Lines changed: 16 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

tests/testthat/test-source-local.R

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
test_that("cpp_source local controls RTTI/vtable symbol visibility", {
2+
skip_on_os("windows")
3+
4+
mk_dirs <- function() {
5+
d1 <- tempfile("cpp_source_local1")
6+
d2 <- tempfile("cpp_source_local2")
7+
dir.create(d1); dir.create(d2)
8+
list(provider = d1, consumer = d2)
9+
}
10+
11+
unload_dirs <- function(dirs) {
12+
dlls <- getLoadedDLLs()
13+
for (nm in names(dlls)) {
14+
# Some R builds/platforms expose different DLL info; access $path
15+
# defensively to avoid errors when the structure differs (macOS).
16+
p <- tryCatch({ dlls[[nm]]$path }, error = function(e) NULL)
17+
if (!is.null(p)) {
18+
for (d in dirs) {
19+
if (grepl(d, p, fixed = TRUE)) {
20+
tryCatch(dyn.unload(p), error = function(e) NULL)
21+
}
22+
}
23+
}
24+
}
25+
}
26+
27+
provider_so_path <- function(dir) {
28+
src <- file.path(dir, "src")
29+
files <- list.files(src, pattern = paste0("\\\\", .Platform$dynlib.ext, "$"), ignore.case = TRUE)
30+
if (length(files) == 0) return(character())
31+
file.path(src, files[[1]])
32+
}
33+
34+
dirs <- mk_dirs()
35+
on.exit({
36+
unload_dirs(unlist(dirs))
37+
unlink(unlist(dirs), recursive = TRUE, force = TRUE)
38+
}, add = TRUE)
39+
40+
# Provider: abstract Base + Impl factory (polymorphic triggers RTTI/vtable)
41+
provider_code <- '
42+
#include <cpp11/R.hpp>
43+
struct Base { virtual ~Base(){}; virtual int foo() = 0; };
44+
struct Impl : Base { int foo() override { return 77; } };
45+
46+
extern "C" Base* make_impl() { return new Impl(); }
47+
extern "C" void destroy_impl(Base* p) { delete p; }
48+
'
49+
50+
# Consumer uses typeid(Base) (forces reference to typeinfo symbol) and
51+
# calls the factory produced by the provider.
52+
consumer_code <- '\n#include <cpp11/R.hpp>\n#include <typeinfo>\n#include <string>\nstruct Base { virtual ~Base(){}; virtual int foo() = 0; };\nextern "C" Base* make_impl();\nextern "C" SEXP call_typeinfo_and_run() {\n const std::type_info& t = typeid(Base);\n std::string n = t.name();\n Base* b = make_impl();\n int v = b->foo();\n delete b;\n SEXP out = PROTECT(Rf_allocVector(INTSXP, 2));\n INTEGER(out)[0] = (int)n.size();\n INTEGER(out)[1] = v;\n UNPROTECT(1);\n return out;\n}\n'
53+
54+
# 1) provider loaded with local = TRUE -> consumer should fail to load
55+
expect_silent(cpp_source(code = provider_code, dir = dirs$provider, clean = FALSE, local = TRUE))
56+
expect_error(
57+
cpp_source(code = consumer_code, dir = dirs$consumer, clean = FALSE),
58+
regexp = "undefined symbol|symbol .* not found|undefined reference|symbol not found in flat namespace",
59+
ignore.case = TRUE
60+
)
61+
62+
# Clean up partial loads
63+
unload_dirs(unlist(dirs))
64+
65+
# 2) provider loaded with local = FALSE -> consumer loads and runs
66+
expect_silent(cpp_source(code = provider_code, dir = dirs$provider, clean = FALSE, local = FALSE))
67+
expect_silent(cpp_source(code = consumer_code, dir = dirs$consumer, clean = FALSE))
68+
69+
res <- .Call("call_typeinfo_and_run")
70+
expect_true(is.integer(res) && length(res) == 2)
71+
expect_equal(as.integer(res)[2], 77L)
72+
expect_true(as.integer(res)[1] > 0)
73+
74+
# Explicit check that the manual dyn.load(...) workaround is unnecessary.
75+
# Emulate the snippet to locate the provider shared object and show that it
76+
# exists; we already demonstrated the consumer works without running this
77+
# manual snippet because cpp_source(local = FALSE) provided global symbols.
78+
# Try to determine the built provider shared object path. On some runners
79+
# the file may not be discoverable via listing (packaged R builds, macOS
80+
# variations). Fall back to inspecting getLoadedDLLs(); if still not
81+
# available, skip the explicit dyn.load check on this platform.
82+
so_path <- provider_so_path(dirs$provider)
83+
if (length(so_path) == 0) {
84+
dlls <- getLoadedDLLs()
85+
for (nm in names(dlls)) {
86+
p <- tryCatch({ dlls[[nm]]$path }, error = function(e) NULL)
87+
if (!is.null(p) && grepl(dirs$provider, p, fixed = TRUE)) {
88+
so_path <- p
89+
break
90+
}
91+
}
92+
}
93+
94+
if (length(so_path) == 0 || !nzchar(so_path)) {
95+
skip("Could not locate provider shared object on this platform; skipping manual dyn.load check")
96+
}
97+
98+
expect_true(file.exists(so_path))
99+
# Loading it manually with local = FALSE would succeed, but wasn't required.
100+
expect_silent(dyn.load(so_path, local = FALSE, now = TRUE))
101+
})

0 commit comments

Comments
 (0)