Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

interface around {fmt} for cpp11::stop() and cpp11::warning()` message formatting #209

Merged
merged 15 commits into from
Jul 22, 2021
Merged

interface around {fmt} for cpp11::stop() and cpp11::warning()` message formatting #209

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2ea819c
added empty method for vecs and updated NEWS
Jun 23, 2021
57beaac
didnt mean to make changes
Jun 23, 2021
c453f48
updating master
Jun 23, 2021
80f4c78
Getting updates for new branch
Jun 24, 2021
2918ebd
getting updates
Jun 25, 2021
0cb2f77
fixing cpp11 decorators bug
Jun 29, 2021
30dbd64
Getting updates for as_double and as_integer
Jul 12, 2021
743c2ca
Get bug fix for cpp_source
Jul 14, 2021
7a8a4d7
format stop and warning with fmt library
Jul 20, 2021
860bd53
added changes to NEWS
Jul 20, 2021
f45593d
fixing checks
Jul 20, 2021
d51a3cd
fixing checks
Jul 20, 2021
1749d62
fixing windows array init error
Jul 21, 2021
e1e8f1b
Update tests for protect
sbearrows Jul 21, 2021
cb2ccca
moved fmt powered stop and warning inside ifdef
Jul 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* Error messages now output original file name rather than the temporary file name (@sbearrows, #194)
* Fixed bug when running `cpp_source()` on the same file more than once (@sbearrows, #202)
* Removed internal instances of `cpp11::stop()` and replaced with C++ exceptions (@sbearrows, #203)
* Added optionally formatting to `stop()` and `warning()` using {fmt} library for better error messages (@sbearrows, #169)

# cpp11 0.3.1

Expand Down
8 changes: 8 additions & 0 deletions cpp11test/R/cpp11.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ data_frame_ <- function() {
.Call(`_cpp11test_data_frame_`)
}

my_stop <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_stop`, mystring, myarg))
}

my_warning <- function(mystring, myarg) {
invisible(.Call(`_cpp11test_my_warning`, mystring, myarg))
}

remove_altrep <- function(x) {
.Call(`_cpp11test_remove_altrep`, x)
}
Expand Down
61 changes: 18 additions & 43 deletions cpp11test/src/cpp11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,22 @@ extern "C" SEXP _cpp11test_data_frame_() {
return cpp11::as_sexp(data_frame_());
END_CPP11
}
// errors.cpp
void my_stop(std::string mystring, int myarg);
extern "C" SEXP _cpp11test_my_stop(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_stop(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<int>>(myarg));
return R_NilValue;
END_CPP11
}
// errors.cpp
void my_warning(std::string mystring, std::string myarg);
extern "C" SEXP _cpp11test_my_warning(SEXP mystring, SEXP myarg) {
BEGIN_CPP11
my_warning(cpp11::as_cpp<cpp11::decay_t<std::string>>(mystring), cpp11::as_cpp<cpp11::decay_t<std::string>>(myarg));
return R_NilValue;
END_CPP11
}
// find-intervals.cpp
SEXP remove_altrep(SEXP x);
extern "C" SEXP _cpp11test_remove_altrep(SEXP x) {
Expand Down Expand Up @@ -321,49 +337,6 @@ extern "C" SEXP _cpp11test_sum_dbl_accumulate2_(SEXP x_sxp) {

extern "C" {
/* .Call calls */
extern SEXP _cpp11test_cpp11_add_vec_for_(SEXP, SEXP);
extern SEXP _cpp11test_cpp11_insert_(SEXP);
extern SEXP _cpp11test_cpp11_release_(SEXP);
extern SEXP _cpp11test_cpp11_safe_(SEXP);
extern SEXP _cpp11test_data_frame_();
extern SEXP _cpp11test_findInterval2(SEXP, SEXP);
extern SEXP _cpp11test_findInterval2_5(SEXP, SEXP);
extern SEXP _cpp11test_findInterval3(SEXP, SEXP);
extern SEXP _cpp11test_findInterval4(SEXP, SEXP);
extern SEXP _cpp11test_gibbs_cpp(SEXP, SEXP);
extern SEXP _cpp11test_gibbs_cpp2(SEXP, SEXP);
extern SEXP _cpp11test_gibbs_rcpp(SEXP, SEXP);
extern SEXP _cpp11test_gibbs_rcpp2(SEXP, SEXP);
extern SEXP _cpp11test_grow_(SEXP);
extern SEXP _cpp11test_protect_many_(SEXP);
extern SEXP _cpp11test_protect_many_cpp11_(SEXP);
extern SEXP _cpp11test_protect_many_preserve_(SEXP);
extern SEXP _cpp11test_protect_many_rcpp_(SEXP);
extern SEXP _cpp11test_protect_many_sexp_(SEXP);
extern SEXP _cpp11test_protect_one_(SEXP, SEXP);
extern SEXP _cpp11test_protect_one_cpp11_(SEXP, SEXP);
extern SEXP _cpp11test_protect_one_preserve_(SEXP, SEXP);
extern SEXP _cpp11test_protect_one_sexp_(SEXP, SEXP);
extern SEXP _cpp11test_rcpp_grow_(SEXP);
extern SEXP _cpp11test_rcpp_release_(SEXP);
extern SEXP _cpp11test_rcpp_sum_dbl_accumulate_(SEXP);
extern SEXP _cpp11test_rcpp_sum_dbl_for_(SEXP);
extern SEXP _cpp11test_rcpp_sum_dbl_foreach_(SEXP);
extern SEXP _cpp11test_rcpp_sum_int_for_(SEXP);
extern SEXP _cpp11test_remove_altrep(SEXP);
extern SEXP _cpp11test_row_sums(SEXP);
extern SEXP _cpp11test_sum_dbl_accumulate2_(SEXP);
extern SEXP _cpp11test_sum_dbl_accumulate_(SEXP);
extern SEXP _cpp11test_sum_dbl_for2_(SEXP);
extern SEXP _cpp11test_sum_dbl_for3_(SEXP);
extern SEXP _cpp11test_sum_dbl_for_(SEXP);
extern SEXP _cpp11test_sum_dbl_foreach2_(SEXP);
extern SEXP _cpp11test_sum_dbl_foreach_(SEXP);
extern SEXP _cpp11test_sum_int_accumulate_(SEXP);
extern SEXP _cpp11test_sum_int_for2_(SEXP);
extern SEXP _cpp11test_sum_int_for_(SEXP);
extern SEXP _cpp11test_sum_int_foreach_(SEXP);
extern SEXP _cpp11test_upper_bound(SEXP, SEXP);
extern SEXP run_testthat_tests(SEXP);

static const R_CallMethodDef CallEntries[] = {
Expand All @@ -381,6 +354,8 @@ static const R_CallMethodDef CallEntries[] = {
{"_cpp11test_gibbs_rcpp", (DL_FUNC) &_cpp11test_gibbs_rcpp, 2},
{"_cpp11test_gibbs_rcpp2", (DL_FUNC) &_cpp11test_gibbs_rcpp2, 2},
{"_cpp11test_grow_", (DL_FUNC) &_cpp11test_grow_, 1},
{"_cpp11test_my_stop", (DL_FUNC) &_cpp11test_my_stop, 2},
{"_cpp11test_my_warning", (DL_FUNC) &_cpp11test_my_warning, 2},
{"_cpp11test_protect_many_", (DL_FUNC) &_cpp11test_protect_many_, 1},
{"_cpp11test_protect_many_cpp11_", (DL_FUNC) &_cpp11test_protect_many_cpp11_, 1},
{"_cpp11test_protect_many_preserve_", (DL_FUNC) &_cpp11test_protect_many_preserve_, 1},
Expand Down
10 changes: 10 additions & 0 deletions cpp11test/src/errors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#include "cpp11/protect.hpp"
using namespace cpp11;

[[cpp11::register]] void my_stop(std::string mystring, int myarg) {
cpp11::stop(mystring, myarg);
}

[[cpp11::register]] void my_warning(std::string mystring, std::string myarg) {
cpp11::warning(mystring, myarg);
}
4 changes: 2 additions & 2 deletions cpp11test/src/test-as.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
#include <testthat.h>

#include <deque>
#include <string>
#include <vector>

#include "cpp11/declarations.hpp"

#include <testthat.h>

#include "Rcpp.h"

context("as_cpp-C++") {
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-data_frame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#include <testthat.h>
#include "cpp11/data_frame.hpp"
#include "cpp11/function.hpp"
#include "cpp11/integers.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>

context("data_frame-C++") {
test_that("data_frame works") {
auto getExportedValue = cpp11::package("base")["getExportedValue"];
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-doubles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
#include <testthat.h>
#include <cstring>
#include "cpp11/doubles.hpp"
#include "cpp11/integers.hpp"
#include "cpp11/sexp.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>

context("doubles-C++") {
test_that("doubles::r_vector(SEXP)") {
cpp11::doubles x(Rf_allocVector(REALSXP, 2));
Expand Down
4 changes: 2 additions & 2 deletions cpp11test/src/test-environment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
#include <testthat.h>

#include "cpp11/as.hpp"
#include "cpp11/environment.hpp"
#include "cpp11/function.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>

context("environment-C++") {
test_that("environment works") {
auto new_env = cpp11::package("base")["new.env"];
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-external_pointer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#include <testthat.h>
#include <iostream>
#include "cpp11/external_pointer.hpp"

#include <testthat.h>

bool deleted = false;

void deleter(int* ptr) {
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-function.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <testthat.h>
#include "cpp11/function.hpp"

#include <testthat.h>

context("function-C++") {
test_that("functions can be called") {
auto median = cpp11::package("stats")["median"];
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-integers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#include <testthat.h>
#include "Rversion.h"
#include "cpp11/doubles.hpp"
#include "cpp11/integers.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>

context("integers-C++") {
test_that("as_integers(doubles)") {
// TYPEOF(x) == INTSXP
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
#include <testthat.h>
#include "cpp11/doubles.hpp"
#include "cpp11/integers.hpp"
#include "cpp11/list.hpp"
#include "cpp11/logicals.hpp"
#include "cpp11/raws.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>

context("list-C++") {
test_that("list.push_back()") {
cpp11::writable::list x;
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-list_of.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#include <testthat.h>
#include "cpp11/doubles.hpp"
#include "cpp11/list.hpp"
#include "cpp11/list_of.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>

context("list_of-C++") {
test_that("list_of works") {
using namespace cpp11::literals;
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-logicals.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <testthat.h>
#include "cpp11/logicals.hpp"

#include <testthat.h>

context("logicals-C++") {
test_that("logicals.push_back()") {
cpp11::writable::logicals x;
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-matrix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#include <testthat.h>
#include "cpp11/doubles.hpp"
#include "cpp11/function.hpp"
#include "cpp11/integers.hpp"
#include "cpp11/matrix.hpp"

#include <testthat.h>

context("matrix-C++") {
test_that("matrix dim attributes are correct for writable matrices") {
cpp11::writable::doubles_matrix x(5, 2);
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-nas.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#include <testthat.h>
#include "cpp11/doubles.hpp"
#include "cpp11/integers.hpp"
#include "cpp11/r_bool.hpp"
#include "cpp11/r_string.hpp"

#include <testthat.h>

context("nas-C++") {
test_that("na integer") { expect_true(cpp11::na<int>() == NA_INTEGER); }
test_that("na double") { expect_true(ISNA(cpp11::na<double>())); }
Expand Down
2 changes: 1 addition & 1 deletion cpp11test/src/test-protect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ context("unwind_protect-C++") {

test_that("stop throws an unwind_exception") {
expect_error_as(cpp11::stop("error"), cpp11::unwind_exception);
expect_error_as(cpp11::stop("error: %s", "message"), cpp11::unwind_exception);
expect_error_as(cpp11::stop("error"), cpp11::unwind_exception);
sbearrows marked this conversation as resolved.
Show resolved Hide resolved
}

test_that("safe wraps R functions and works if there is an R error") {
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-raws.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <testthat.h>
#include "cpp11/raws.hpp"

#include <testthat.h>

context("raws-C++") {
test_that("raws.push_back()") {
cpp11::writable::raws x;
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-sexp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <testthat.h>
#include "cpp11/list.hpp"

#include <testthat.h>

context("sexp-C++") {
test_that("sexp initializer lists work") {
using namespace cpp11::literals;
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-string.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <testthat.h>
#include "cpp11/strings.hpp"

#include <testthat.h>

context("string-C++") {
test_that("is_na(string)") {
cpp11::r_string x("foo");
Expand Down
3 changes: 2 additions & 1 deletion cpp11test/src/test-strings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
#include <testthat.h>
#include <cstring>
#include "cpp11/sexp.hpp"
#include "cpp11/strings.hpp"

#include <testthat.h>

context("strings-C++") {
test_that("strings.push_back()") {
cpp11::writable::strings x;
Expand Down
15 changes: 15 additions & 0 deletions cpp11test/tests/testthat/test_formatted_errors.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
test_that("cpp11::stop formatting works", {
test1 <- 4
expect_error(my_stop("Your number is {}", test1), "Your number is 4", fixed = TRUE)

test2 <- c(3, 5, 7)
expect_error(my_stop("You've tested this {} times", test2[1]), "You've tested this 3 times",
fixed = TRUE)
})
test_that("cpp11::warning formatting works", {
test1 <- "warning"
expect_warning(my_warning("This is a {}", test1), "This is a warning", fixed = TRUE)

test2 <- c("failed", "passed")
expect_warning(my_warning("You {}", test2[2]), "You passed", fixed = TRUE)
})
23 changes: 15 additions & 8 deletions inst/include/cpp11/protect.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
#include "R_ext/Utils.h" // for R_CheckUserInterrupt
#include "Rversion.h" // for R_VERSION, R_Version

#define FMT_HEADER_ONLY
#include "fmt/core.h"

#if defined(R_VERSION) && R_VERSION >= R_Version(3, 5, 0)
#define HAS_UNWIND_PROTECT
#endif
Expand Down Expand Up @@ -195,23 +198,27 @@ constexpr struct protect safe = {};
inline void check_user_interrupt() { safe[R_CheckUserInterrupt](); }

template <typename... Args>
void stop [[noreturn]] (const char* fmt, Args... args) {
safe.noreturn(Rf_errorcall)(R_NilValue, fmt, args...);
void stop [[noreturn]] (const char* fmt_arg, Args... args) {
bkietz marked this conversation as resolved.
Show resolved Hide resolved
std::string msg = fmt::format(fmt_arg, args...);
jimhester marked this conversation as resolved.
Show resolved Hide resolved
safe.noreturn(Rf_errorcall)(R_NilValue, "%s", msg.c_str());
}

template <typename... Args>
void stop [[noreturn]] (const std::string& fmt, Args... args) {
safe.noreturn(Rf_errorcall)(R_NilValue, fmt.c_str(), args...);
void stop [[noreturn]] (const std::string& fmt_arg, Args... args) {
std::string msg = fmt::format(fmt_arg, args...);
Copy link
Collaborator

@bkietz bkietz Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it's unfortunate that we're losing the %x format specifier for SEXPs. This could be recoverable by introducing a helper function:

// pass through by default
template <typename T>
const T& as_formattable(const T& value) { return value; }

// specialization for SEXP: 
std::string as_formattable(SEXP sexp) { return gettextf("%x", sexp); }

Used like so:

Suggested change
std::string msg = fmt::format(fmt_arg, args...);
std::string msg = fmt::format(fmt_arg, as_formattable(args)...);

Copy link
Member

@jimhester jimhester Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you would use cpp11::stop("{0:x}", my_sexp), or ({0:#x} if you want the leading 0x) to print the hexadecimal address for a sexp. (https://fmt.dev/latest/syntax.html#format-examples)

Copy link
Collaborator

@bkietz bkietz Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I misunderstood the purpose of %x. I thought it gave a string representation of the pointed value rather than a representation of the pointer itself. Let me rephrase: would it be desirable to intercept SEXP pointers and replace them with capture.output(print(value)) or so? It'd probably be better to include that as a sexp::to_string() so that we can write

cpp11::stop("Invalid: {}", cpp11::sexp(value).to_string());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, maybe, though it is not always clear what output would be preferred for a given SEXP, possibly something like that returned from R_inspect() might be the most useful for any SEXP type, though the default arguments can generate a lot of data for deeply nested objects.

> .Internal(inspect(1:10))
@7ff6163c1388 13 INTSXP g0c0 [REF(65535)]  1 : 10 (compact)

safe.noreturn(Rf_errorcall)(R_NilValue, "%s", msg.c_str());
}

template <typename... Args>
void warning(const char* fmt, Args... args) {
safe[Rf_warningcall](R_NilValue, fmt, args...);
void warning(const char* fmt_arg, Args... args) {
std::string msg = fmt::format(fmt_arg, args...);
safe[Rf_warningcall](R_NilValue, "%s", msg.c_str());
}

template <typename... Args>
void warning(const std::string& fmt, Args... args) {
safe[Rf_warningcall](R_NilValue, fmt.c_str(), args...);
void warning(const std::string& fmt_arg, Args... args) {
std::string msg = fmt::format(fmt_arg, args...);
safe[Rf_warningcall](R_NilValue, "%s", msg.c_str());
}

/// A doubly-linked list of preserved objects, allowing O(1) insertion/release of
Expand Down
Loading