diff --git a/.github/PATCHSTACK.md b/.github/PATCHSTACK.md new file mode 100644 index 00000000..f8f50aee --- /dev/null +++ b/.github/PATCHSTACK.md @@ -0,0 +1,77 @@ +# The patch stack + +This is a fork of [r-lib/cpp11](https://github.com/r-lib/cpp11) that carries a +handful of changes upstream does not ship. +It is maintained as a *patch stack*: +one branch per change, +each a single commit on top of upstream, +and one branch that is upstream plus all of them. + +## The branches + +- **`main`** is a 1:1 mirror of `r-lib/cpp11`'s `main`. + The [Pull app](https://pull.git.ci/) hard-resets it on every sync + ([`pull.yml`](pull.yml)), + so a commit made here is discarded the next time upstream moves. + Never commit to it. +- **`fork`** is the default branch and the one to install: + `main` plus every patch, squashed in lexicographic branch order. + It is rebuilt from scratch on each sync — never commit to it either, + and never merge into it. +- **`a-fork-infra`**, **`b-*`**, **`f-*`** are the patch branches, + each based on `main`. + `a-` is this fork's own infrastructure (it sorts first, so it lands first), + `b-` is a bug fix, `f-` is a feature. + [`patchstack-sync.yml`](workflows/patchstack-sync.yml) picks up + `b-*` and `f-*` by glob, + so a working branch under any other name is left alone. + +## The sync + +[`patchstack-sync.yml`](workflows/patchstack-sync.yml) runs +[krlmlr/patchstack](https://github.com/krlmlr/patchstack) nightly, +and on demand from the Actions tab. +Each run replays every patch branch onto the current `main`, +squashes them into a fresh `fork` in branch order, +and pushes the lot atomically. + +A patch that no longer applies is left at its old commit +and dropped from that run's `fork`, +so one broken patch never blocks the others — +resolve it by rebasing that branch onto `main` by hand and pushing it. +A patch whose change has landed upstream replays to nothing; +its branch is deleted and the disposition recorded in `refs/notes/patchstack`. + +## Adding a patch + +```bash +git fetch origin +git switch -c f-my-change origin/main +# ... one commit ... +git push -u origin f-my-change +``` + +The next sync folds it into `fork`. +Keep it to a single commit where you can: +the stack is easier to read, +and the squash into `fork` is what everyone consumes anyway. + +Each patch is a candidate for upstream. +Opening a pull request from its branch against `r-lib/cpp11` +costs nothing here — the branch stays exactly where the sync expects it — +and a merged patch cleans itself up on the following run. + +## Installing + +```r +pak::pak("krlmlr/cpp11") +``` + +`fork` is the default branch, so this installs the whole stack. +It is also built by +[krlmlr.r-universe.dev](https://krlmlr.r-universe.dev), +which is the faster route: + +```r +install.packages("cpp11", repos = c("https://krlmlr.r-universe.dev", getOption("repos"))) +``` diff --git a/.github/pull.yml b/.github/pull.yml new file mode 100644 index 00000000..1162ef0a --- /dev/null +++ b/.github/pull.yml @@ -0,0 +1,26 @@ +# Configuration for the "Pull" GitHub App, which keeps this fork's `main` in sync +# with the upstream r-lib/cpp11 repository. +# +# Pointers: +# - App / install / manage: https://pull.git.ci/ +# - Source: https://github.com/wei/pull +# - Configuration reference: https://github.com/wei/pull/blob/master/docs/CONFIGURATIONS.md +# - Trigger a manual sync: https://pull.git.ci/process/krlmlr/cpp11 +# +# Notes: +# - This file must live on the fork's default branch (`fork`) for Pull to read it. +# - `main` is a 1:1 mirror of r-lib/cpp11's `main`, so `hardreset` is the only +# correct merge method: any commit made on it here is discarded on the next sync. +# Nothing may be committed to `main` -- the patch branches carry this fork's work. +# - `fork` has no rule on purpose: it is not a mirror but `main` plus the squashed +# patch stack, and .github/workflows/patchstack-sync.yml rebuilds it once Pull +# has moved `main` forward. +# - Deleting this file would not disable Pull: without a configuration it hard-resets +# the fork's default branch from upstream, which is exactly what `fork` must not do. + +version: "1" + +rules: + - base: main + upstream: r-lib:main + mergeMethod: hardreset diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index a4aafd29..22703f58 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -6,9 +6,11 @@ # usethis::use_github_action("check-standard") will install it. on: push: - branches: [main, master] + # `fork` is this fork's integration branch, rebuilt from `main` plus the + # patch stack; checking it is what says the stack still builds. + branches: [main, master, fork] pull_request: - branches: [main, master] + branches: [main, master, fork] name: R-CMD-check diff --git a/.github/workflows/patchstack-sync.yml b/.github/workflows/patchstack-sync.yml new file mode 100644 index 00000000..ecfb6829 --- /dev/null +++ b/.github/workflows/patchstack-sync.yml @@ -0,0 +1,63 @@ +name: Patchstack sync + +# Rebuilds `fork` -- this repository's default branch -- as `main` plus the +# squashed patch stack, replaying every patch branch onto `main` first. +# +# `main` is a 1:1 mirror of r-lib/cpp11, hard-reset by the Pull app +# (see .github/pull.yml), so it is the upstream this sync tracks and there is no +# second remote to fetch. The patch branches are the `b-*` (bug fix) and `f-*` +# (feature) branches; everything else in this repository is left alone. +# +# The push needs a token that may write workflow files, because `fork` carries +# this file and .github/pull.yml through the `a-fork-infra` patch. GITHUB_TOKEN +# may not, and it only matters when their content changes -- a sync that leaves +# them untouched goes through with the default token. Set PATCHSTACK_TOKEN to a +# fine-grained PAT with Contents: read and write and Workflows: read and write +# to make the exception unnecessary. + +on: + schedule: + # Off the hour: runs scheduled at :00 queue behind everyone else's. + - cron: "23 4 * * *" + workflow_dispatch: + repository_dispatch: + types: [main-updated] + +concurrency: + group: patchstack-sync + cancel-in-progress: false + +permissions: + contents: read + +jobs: + sync: + name: Rebuild the fork branch + runs-on: ubuntu-latest + timeout-minutes: 20 + + permissions: + contents: write + + steps: + - name: Check out this repository + uses: actions/checkout@v6 + with: + # The replay needs every patch branch's history; blobs come on demand. + fetch-depth: 0 + filter: blob:none + token: ${{ secrets.PATCHSTACK_TOKEN || github.token }} + + - name: Check out patchstack + uses: actions/checkout@v6 + with: + repository: krlmlr/patchstack + path: .patchstack + persist-credentials: false + + - name: Sync + uses: ./.patchstack + with: + main_branch: fork + upstream_branch: main + patch_glob: "b-* f-*" diff --git a/NEWS.md b/NEWS.md index f7e08809..98252a46 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # cpp11 (development version) +* `cpp_register()` now also picks up a `{package}_types.h` or `{package}_types.hpp` in `src/include/`, letting a package keep that header private instead of installing it from `inst/include/`. + # cpp11 0.5.5 * Fixed an issue where `cpp11::stop()` and `cpp11::warning()` calls with the same template instantiation could cause a crash on some systems (#491, #295). diff --git a/R/register.R b/R/register.R index 68c7f585..ef1de4de 100644 --- a/R/register.R +++ b/R/register.R @@ -61,7 +61,7 @@ cpp_register <- function( funs <- get_registered_functions(all_decorations, "cpp11::register", quiet) package <- desc::desc_get("Package", file = file.path(path, "DESCRIPTION")) - package <- sub("[.]", "_", package) + package <- gsub("[.]", "_", package) cpp_functions_definitions <- generate_cpp_functions(funs, package) @@ -114,17 +114,28 @@ cpp_register <- function( ) } + pkg_types_name <- paste0(package, c("_types.h", "_types.hpp")) + pkg_types <- c( - file.path(path, "src", paste0(package, "_types.h")), - file.path(path, "src", paste0(package, "_types.hpp")), - file.path(path, "inst", "include", paste0(package, "_types.h")), - file.path(path, "inst", "include", paste0(package, "_types.hpp")) + file.path(path, "src", pkg_types_name), + file.path(path, "src", "include", pkg_types_name), + file.path(path, "inst", "include", pkg_types_name) + ) + + # `src/cpp11.cpp` is generated next to the `src/` copies and compiled from + # `src/`, so a header in `src/include/` is included through that directory. + # A package keeping its own headers private this way then needs no include + # flag for them, and none of the other two locations changes. + pkg_types_include <- c( + pkg_types_name, + file.path("include", pkg_types_name), + pkg_types_name ) pkg_types_exist <- file.exists(pkg_types) if (any(pkg_types_exist)) { extra_includes <- c( - sprintf('#include "%s"', basename(pkg_types[pkg_types_exist])), + sprintf('#include "%s"', pkg_types_include[pkg_types_exist]), extra_includes ) } diff --git a/R/vendor.R b/R/vendor.R index 1d7dc7d2..81de3c47 100644 --- a/R/vendor.R +++ b/R/vendor.R @@ -16,6 +16,11 @@ #' code until you run `cpp_vendor()` again. #' #' @inheritParams cpp_register +#' @param date The date recorded in the `vendored on:` header of each vendored +#' file. Defaults to the current date; pass a fixed date to make vendoring +#' reproducible. +#' @param overwrite If `TRUE`, an existing vendored copy is removed first +#' instead of raising an error. #' @return The file path to the vendored code (invisibly). #' @export #' @examples @@ -30,18 +35,22 @@ #' #' # cleanup #' unlink(dir, recursive = TRUE) -cpp_vendor <- function(path = ".") { +cpp_vendor <- function(path = ".", date = Sys.Date(), overwrite = FALSE) { new <- file.path(path, "inst", "include", "cpp11") if (dir.exists(new)) { - stop( - "'", - new, - "' already exists\n * run unlink('", - new, - "', recursive = TRUE)", - call. = FALSE - ) + if (overwrite) { + unlink(new, recursive = TRUE) + } else { + stop( + "'", + new, + "' already exists\n * run unlink('", + new, + "', recursive = TRUE)", + call. = FALSE + ) + } } dir.create(new, recursive = TRUE, showWarnings = FALSE) @@ -56,7 +65,7 @@ cpp_vendor <- function(path = ".") { cpp11_header <- sprintf( "// cpp11 version: %s\n// vendored on: %s", cpp11_version, - Sys.Date() + as.Date(date) ) files <- list.files(current, full.names = TRUE) diff --git a/cpp11test/src/test-external_pointer.cpp b/cpp11test/src/test-external_pointer.cpp index 897fd4f2..d8b960aa 100644 --- a/cpp11test/src/test-external_pointer.cpp +++ b/cpp11test/src/test-external_pointer.cpp @@ -36,4 +36,53 @@ context("external_pointer-C++") { uniq.reset(); expect_true(deleted == true); } + + test_that("external_pointer preserves attributes when moved (issue #308)") { + // Test move constructor + { + int* value = new int(42); + cpp11::external_pointer p(value); + + // Set an attribute on the external pointer + Rf_setAttrib(p, R_ClassSymbol, Rf_mkString("test_class")); + + // Verify attribute exists before move + SEXP class_attr = Rf_getAttrib(p, R_ClassSymbol); + expect_true(class_attr != R_NilValue); + + // Move the external pointer using move constructor + cpp11::external_pointer p_moved = std::move(p); + + // Verify attribute is preserved after move + SEXP class_attr_after = Rf_getAttrib(p_moved, R_ClassSymbol); + expect_true(class_attr_after != R_NilValue); + expect_true(strcmp(CHAR(STRING_ELT(class_attr_after, 0)), "test_class") == 0); + + // Clean up + delete p_moved.release(); + } + + // Test move assignment operator + { + int* value1 = new int(1); + cpp11::external_pointer p1(value1); + + // Set an attribute on p1 + Rf_setAttrib(p1, R_ClassSymbol, Rf_mkString("test_class")); + + // Create p2 with nullptr (no memory leak) + cpp11::external_pointer p2(nullptr); + + // Move assign p1 to p2 + p2 = std::move(p1); + + // Verify attribute is preserved after move assignment + SEXP class_attr_after = Rf_getAttrib(p2, R_ClassSymbol); + expect_true(class_attr_after != R_NilValue); + expect_true(strcmp(CHAR(STRING_ELT(class_attr_after, 0)), "test_class") == 0); + + // Clean up + delete p2.release(); + } + } } diff --git a/inst/include/cpp11/declarations.hpp b/inst/include/cpp11/declarations.hpp index ed57721f..89f24a65 100644 --- a/inst/include/cpp11/declarations.hpp +++ b/inst/include/cpp11/declarations.hpp @@ -41,7 +41,7 @@ T& unmove(T&& t) { SEXP err = R_NilValue; \ char buf[CPP11_ERROR_BUFSIZE] = ""; \ try { -#define END_CPP11 \ +#define END_CPP11_EX(RET) \ } \ catch (cpp11::unwind_exception & e) { \ err = e.token; \ @@ -57,4 +57,5 @@ T& unmove(T&& t) { } else if (err != R_NilValue) { \ R_ContinueUnwind(err); \ } \ - return R_NilValue; + return RET; +#define END_CPP11 END_CPP11_EX(R_NilValue) diff --git a/inst/include/cpp11/external_pointer.hpp b/inst/include/cpp11/external_pointer.hpp index a62134ec..61b7b50c 100644 --- a/inst/include/cpp11/external_pointer.hpp +++ b/inst/include/cpp11/external_pointer.hpp @@ -55,8 +55,9 @@ class external_pointer { external_pointer(SEXP data) : data_(valid_type(data)) {} - external_pointer(pointer p, bool use_deleter = true, bool finalize_on_exit = true) - : data_(safe[R_MakeExternalPtr]((void*)p, R_NilValue, R_NilValue)) { + external_pointer(pointer p, bool use_deleter = true, bool finalize_on_exit = true, + SEXP prot = R_NilValue) + : data_(safe[R_MakeExternalPtr]((void*)p, R_NilValue, prot)) { if (use_deleter) { R_RegisterCFinalizerEx(data_, r_deleter, static_cast(finalize_on_exit)); } @@ -66,9 +67,19 @@ class external_pointer { data_ = safe[Rf_shallow_duplicate](rhs.data_); } - external_pointer(external_pointer&& rhs) { reset(rhs.release()); } + external_pointer(external_pointer&& rhs) { + data_ = rhs.data_; + rhs.data_ = R_NilValue; + } - external_pointer& operator=(external_pointer&& rhs) noexcept { reset(rhs.release()); } + external_pointer& operator=(external_pointer&& rhs) noexcept { + // This works even if `this == &rhs` because `data_` (a `cpp11::sexp`) handles + // the underlying resource. + data_ = rhs.data_; + // Order matters: first assign, then clear the RHS. + rhs.data_ = R_NilValue; + return *this; + } external_pointer& operator=(std::nullptr_t) noexcept { reset(); }; diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index e3b8ce81..2a92aeec 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -247,14 +247,10 @@ void stop [[noreturn]] (const std::string& fmt_arg, Args&&... args) { safe.noreturn(Rf_errorcall)(R_NilValue, "%s", msg.c_str()); } -template -void warning(const char* fmt_arg, Args&&... args) { - std::string msg = fmt::format(fmt::runtime(fmt_arg), std::forward(args)...); - safe[Rf_warningcall](R_NilValue, "%s", msg.c_str()); -} +// Always making a copy of the string to avoid weird unwind behavior. template -void warning(const std::string& fmt_arg, Args&&... args) { +void warning(const std::string fmt_arg, Args&&... args) { std::string msg = fmt::format(fmt::runtime(fmt_arg), std::forward(args)...); safe[Rf_warningcall](R_NilValue, "%s", msg.c_str()); } @@ -269,13 +265,10 @@ void stop [[noreturn]] (const std::string& fmt, Args... args) { safe.noreturn(Rf_errorcall)(R_NilValue, fmt.c_str(), args...); } -template -void warning(const char* fmt, Args... args) { - safe[Rf_warningcall](R_NilValue, fmt, args...); -} +// Always making a copy of the string to avoid weird unwind behavior. template -void warning(const std::string& fmt, Args... args) { +void warning(const std::string fmt, Args... args) { safe[Rf_warningcall](R_NilValue, fmt.c_str(), args...); } #endif diff --git a/inst/include/cpp11/r_vector.hpp b/inst/include/cpp11/r_vector.hpp index 317b16f1..3acde389 100644 --- a/inst/include/cpp11/r_vector.hpp +++ b/inst/include/cpp11/r_vector.hpp @@ -270,6 +270,7 @@ class r_vector : public cpp11::r_vector { public: proxy(SEXP data, const R_xlen_t index, underlying_type* const p, bool is_altrep); + proxy(const proxy&) = default; proxy& operator=(const proxy& rhs); diff --git a/inst/include/cpp11/sexp.hpp b/inst/include/cpp11/sexp.hpp index 34f7e2b8..70927c95 100644 --- a/inst/include/cpp11/sexp.hpp +++ b/inst/include/cpp11/sexp.hpp @@ -67,11 +67,20 @@ class sexp { SEXP data() const { return data_; } /// DEPRECATED: Do not use this, it will be removed soon. - operator double() const { return REAL_ELT(data_, 0); } + [[deprecated("Implicit conversion from cpp11::sexp to double is deprecated")]] + operator double() const { + return REAL_ELT(data_, 0); + } /// DEPRECATED: Do not use this, it will be removed soon. - operator size_t() const { return REAL_ELT(data_, 0); } + [[deprecated("Implicit conversion from cpp11::sexp to size_t is deprecated")]] + operator size_t() const { + return REAL_ELT(data_, 0); + } /// DEPRECATED: Do not use this, it will be removed soon. - operator bool() const { return LOGICAL_ELT(data_, 0); } + [[deprecated("Implicit conversion from cpp11::sexp to bool is deprecated")]] + operator bool() const { + return LOGICAL_ELT(data_, 0); + } }; } // namespace cpp11 diff --git a/man/cpp_vendor.Rd b/man/cpp_vendor.Rd index 857e49cf..6e88fd1e 100644 --- a/man/cpp_vendor.Rd +++ b/man/cpp_vendor.Rd @@ -4,10 +4,17 @@ \alias{cpp_vendor} \title{Vendor the cpp11 dependency} \usage{ -cpp_vendor(path = ".") +cpp_vendor(path = ".", date = Sys.Date(), overwrite = FALSE) } \arguments{ \item{path}{The path to the package root directory} + +\item{date}{The date recorded in the \code{vendored on:} header of each vendored +file. Defaults to the current date; pass a fixed date to make vendoring +reproducible.} + +\item{overwrite}{If \code{TRUE}, an existing vendored copy is removed first +instead of raising an error.} } \value{ The file path to the vendored code (invisibly). diff --git a/tests/testthat/test-register.R b/tests/testthat/test-register.R index 7f65bb8a..7e3c86b6 100644 --- a/tests/testthat/test-register.R +++ b/tests/testthat/test-register.R @@ -685,6 +685,50 @@ describe("cpp_register", { ) }) + it("includes pkg_types.h if included in src/include", { + pkg <- local_package() + p <- pkg_path(pkg) + dir.create(file.path(p, "src", "include"), recursive = TRUE) + file.copy(test_path("single.cpp"), file.path(p, "src", "single.cpp")) + writeLines( + "#include ", + file.path(p, "src", "include", "testPkg_types.h") + ) + cpp_register(p) + + expect_true( + any( + grepl( + pattern = '#include "include/testPkg_types.h"', + x = readLines(file.path(p, "src", "cpp11.cpp")), + fixed = TRUE + ) + ) + ) + }) + + it("includes pkg_types.hpp if included in src/include", { + pkg <- local_package() + p <- pkg_path(pkg) + dir.create(file.path(p, "src", "include"), recursive = TRUE) + file.copy(test_path("single.cpp"), file.path(p, "src", "single.cpp")) + writeLines( + "#include ", + file.path(p, "src", "include", "testPkg_types.hpp") + ) + cpp_register(p) + + expect_true( + any( + grepl( + pattern = '#include "include/testPkg_types.hpp"', + x = readLines(file.path(p, "src", "cpp11.cpp")), + fixed = TRUE + ) + ) + ) + }) + it("includes pkg_types.h if included in inst/include", { pkg <- local_package() p <- pkg_path(pkg)