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

Add missing Allocator template argument for basic_memory_buffer in fo… #2300

Merged
merged 1 commit into from
May 21, 2021

Conversation

voxmea
Copy link
Contributor

@voxmea voxmea commented May 21, 2021

…rmat_to

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

typename Char = enable_if_t<detail::is_string<S>::value, char_t<S>>>
inline auto format_to(basic_memory_buffer<Char, SIZE>& buf, const S& format_str,
typename Char = enable_if_t<detail::is_string<S>::value, char_t<S>>,
typename Allocator = std::allocator<Char>>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that Allocator shouldn't be defaulted because it's deduced from the argument. Otherwise looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, isn't that true for all of basic_memory_buffer's template arguments here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, the defaults means I can pass {} and get the defaulted values. I think that means I can remove the default SIZE then.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true for SIZE (could you remove the default while at it?) but not for Char which is used for SFINAE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, but it must be an lvalue. Sorry for the stream of thought here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed another version that removes the default template argument for vformat_to. Let me know if you want me to limit the change to just format_to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please limit the changes to format_to.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you see the default argument to Char being used?

I guess something like:

format_to<std::basic_string<wchar_t>>(...)

Copy link
Contributor

Choose a reason for hiding this comment

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

But it's still deduced from buf, right? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to limit the change to format_to.

@voxmea voxmea force-pushed the master branch 6 times, most recently from cedfe6e to 967c394 Compare May 21, 2021 18:32
template <typename S, typename... Args, typename Char, size_t SIZE,
typename Allocator, FMT_ENABLE_IF(detail::is_string<S>::value)>
inline auto format_to(basic_memory_buffer<Char, SIZE, Allocator>& buf,
const S& format_str,
Args&&... args) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Does clang-format still break this onto the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks, this time run through clang-format.

…rmat_to

Remove deduced default template arguments in format_to and moves the
SFINAE check to a non-deduced template parameter.
@vitaut vitaut merged commit 5c4b0c8 into fmtlib:master May 21, 2021
@vitaut
Copy link
Contributor

vitaut commented May 21, 2021

Thank you

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.

3 participants