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

Provide more overloads for the wide string flavour #724

Closed
wants to merge 1 commit into from

Conversation

DanielaE
Copy link
Contributor

Signed-off-by: Daniela Engert dani@ngrt.de

@DanielaE
Copy link
Contributor Author

DanielaE commented Apr 30, 2018

This code compiles now and is working fine on my machine:

auto sResult = fmt::sprintf("Elapsed time: %.2f seconds", 1.23);
auto wResult = fmt::sprintf(L"Elapsed time: %.2f seconds", 1.23);
fmt::print(std::cout, "Elapsed time: {:.2f} {}\n", 1.23, "seconds");
fmt::print(std::wcout, L"Elapsed time: {:.2f} {}\n", 1.23, L"seconds");

fmt::print("Elapsed time: {:.2f} {}\n", 1.23, "seconds");
fmt::print_colored(fmt::green, "Elapsed time: {:.2f} {}{}\n", 1.23, 'm', "seconds");
fmt::print(stdout, "Elapsed time: {:.2f} {}\n", 1.23, "seconds");
auto n = fmt::printf("Elapsed time: %.2f %cseconds\n", 1.23, 'm');
n = fmt::fprintf(stdout, "Elapsed time: %.2f %cseconds\n", 1.23, 'm');

_setmode(_fileno(stdout), _O_U16TEXT); // switch stdout into wchar_t (UTF16) mode

fmt::print(L"Elapsed time: {:.2f} {}\n", 1.23, L"seconds");
fmt::print_colored(fmt::green, L"Elapsed time: {:.2f} {}{}\n", 1.23, L'm', L"seconds");
fmt::print(stdout, L"Elapsed time: {:.2f} {}\n", 1.23, L"seconds");
n = fmt::printf(L"Elapsed time: %.2f %cseconds\n", 1.23, L'm');
n = fmt::fprintf(stdout, L"Elapsed time: %.2f %cseconds\n", 1.23, L'm');

Should handle issue #721

@DanielaE DanielaE force-pushed the feature/more-wide-overloads branch 3 times, most recently from 863141c to 04ba6fc Compare April 30, 2018 12:37
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 a lot for the PR! Looks good, but could you add a few unit tests for the new functions?

@@ -1093,12 +1093,18 @@ class basic_format_args {

/** An alias to ``basic_format_args<context>``. */
// It is a separate type rather than a typedef to make symbols readable.
// It is also required to disambiguate 'vformat_to' overloads
// in core.h and format.h - which seems a bit brittle!
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate on the disambiguation part and why you think it's brittle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Victor,

this is probably no longer true. During my various attempts, to replace

template <typename Container>
... vformat_to(std::back_insert_iterator<Container> out,
               string_view format_str, format_args args) {
...
}

in core.h by

template <typename Container>
... vformat_to(std::back_insert_iterator<Container> out,
      basic_string_view<typename Container::value_type> format_str,
      basic_format_args<typename buffer_context<typename Container::value_type>::type> args) {
...
}

to reduce code duplication, I got ambiguous overloads of vformat_toin core.h and format.h. My tests from today show that those more-derived structs don't help me to get along with this problem. Sigh ...

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is not so much due to basic_format_args, but due to overloads taking string_view in format.h. Unfortunately I don't think it's possible to parameterize on character type there: https://godbolt.org/g/bPevXe.

this is probably no longer true

I've removed the comment then.

@DanielaE DanielaE force-pushed the feature/more-wide-overloads branch from 04ba6fc to 4350f1c Compare May 2, 2018 16:26
Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE DanielaE force-pushed the feature/more-wide-overloads branch from 4350f1c to d735473 Compare May 2, 2018 16:27
@DanielaE
Copy link
Contributor Author

DanielaE commented May 2, 2018

Do you need more test cases? And if so, where?

@vitaut
Copy link
Contributor

vitaut commented May 5, 2018

Do you need more test cases? And if so, where?

Thanks a lot for adding tests, I think these are fine for now.

Merged in 2570f1a with minor tweaks.

@vitaut vitaut closed this May 5, 2018
@DanielaE DanielaE deleted the feature/more-wide-overloads branch May 5, 2018 16:30
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.

2 participants