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

Doc: Unclear contract for retention of fmt::arg results #2178

Closed
BillyDonahue opened this issue Mar 13, 2021 · 3 comments
Closed

Doc: Unclear contract for retention of fmt::arg results #2178

BillyDonahue opened this issue Mar 13, 2021 · 3 comments

Comments

@BillyDonahue
Copy link
Contributor

BillyDonahue commented Mar 13, 2021

Docs for fmt::arg() seem to indicate that the returned object my depend on temporaries created in the function call expression, and that it would be unsafe to use the returned object after the full expression expires.

fmt/include/fmt/core.h

Lines 1622 to 1623 in b8ff3c1

Returns a named argument to be used in a formatting function. It should only
be used in a call to a formatting function.

That's no longer true as other examples like

fmt/include/fmt/args.h

Lines 180 to 181 in b8ff3c1

auto a1 = fmt::arg("a1_", a1_val);
store.push_back(std::cref(a1));
show the result being retained in auto variables and passed to fmt::dynamic_format_arg_store via push_back.

I want to use dynamic_format_arg_store but it looks like I'm breaking the fmt::arg contract to do it.
I know it's safe, but the docs don't quite match the reality.

Should the "It should only be used in a call to a formatting function" be removed or updated?

@vitaut
Copy link
Contributor

vitaut commented Mar 14, 2021

Good catch, thanks!

The example in fmt/args.h is pretty bad, we shouldn't be storing the return value of fmt::args. I've updated the comments in 5a1127b and disabled wrapping named arguments in cref since it's misleading (argument name is always stored by reference) and doesn't work with rvalue references.

@vitaut vitaut closed this as completed Mar 14, 2021
@BillyDonahue
Copy link
Contributor Author

A new comment says "The name is always stored by reference", but it is actually copied into the dynamic_args_ as a std::basic_string<char_type>.

fmt/include/fmt/args.h

Lines 190 to 205 in 6151d0d

/**
Adds named argument into the dynamic store for later passing to a formatting
function. ``std::reference_wrapper`` is supported to avoid copying of the
argument. The name is always stored by reference.
*/
template <typename T>
void push_back(const detail::named_arg<char_type, T>& arg) {
const char_type* arg_name =
dynamic_args_.push<std::basic_string<char_type>>(arg.name).c_str();
if (detail::const_check(need_copy<T>::value)) {
emplace_arg(
fmt::arg(arg_name, dynamic_args_.push<stored_type<T>>(arg.value)));
} else {
emplace_arg(fmt::arg(arg_name, arg.value));
}
}

Maybe it's hard to see because arg_name looks like arg.name.

@vitaut
Copy link
Contributor

vitaut commented Mar 15, 2021

You are right. Fixed in 8308f52.

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

No branches or pull requests

2 participants