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.

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

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

Merged
merged 15 commits into from
Jul 22, 2021

Conversation

sbearrows
Copy link
Contributor

@jimhester Let me know what you think! Tests are pretty simple but we can add more if issues arise.

Fixes #169

@jimhester
Copy link
Member

So I was thinking about this some this morning, and I think we will either have to use new function names (something like stop_fmt() for the fmt powered functions, or put them behind a #ifdef CPP11_USE_FMT flag that you have to opt into to use them.

This is because existing packages that are using cpp11::stop() are going to be using the sprintf style %s etc. message strings, and these won't work with fmt.

Another argument in favor of the #ifdef approach is that compiling the fmt code will incur some overhead, which some people may want to avoid.

Which way do you think would be best?

@sbearrows
Copy link
Contributor Author

I'd be in favor of the later! I think decreasing overhead is probably important to a lot of people using cpp11. What about you?

@jimhester
Copy link
Member

yeah that is the way I was leaning, though I worry if it is only opt-in then people may not realize it is an option / never use it.

@sbearrows
Copy link
Contributor Author

We could try to make a ruckus about? Adding examples in documentation?

Also, could we make fmt for cpp11::stop() and cpp11::warning() opt-in but make fmt for the new cpp11::messages() opt-out since it isn't present in previous versions? This might give people some breadcrumbs for the others? Not sure if that would be too confusing though.

cpp11test/src/test-protect.cpp Outdated Show resolved Hide resolved
inst/include/cpp11/protect.hpp Outdated Show resolved Hide resolved
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

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

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

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)

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
inst/include/cpp11/protect.hpp Outdated Show resolved Hide resolved
@jimhester
Copy link
Member

LGTM, I think this can be merged once the tests are green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an interface around {fmt} for easier messaging
3 participants