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

Add file stream support for stylized text printing. #967

Merged
merged 4 commits into from
Dec 9, 2018
Merged

Add file stream support for stylized text printing. #967

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23db91a
Addressed review comments.
Rakete1111 Dec 7, 2018
c085401
Add some unit tests.
Rakete1111 Dec 7, 2018
a690029
Add a brief comment explaining why the overload is needed.
Rakete1111 Dec 9, 2018
f7f0b0e
Move disable-FILE-sfinae to color.h
Rakete1111 Dec 9, 2018
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
43 changes: 29 additions & 14 deletions include/fmt/color.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,39 +427,54 @@ inline void reset_color<wchar_t>(FILE *stream) FMT_NOEXCEPT {

template <
typename S, typename Char = typename internal::char_t<S>::type>
void vprint(const text_style &tf, const S &format,
void vprint(std::FILE *f, const text_style &ts, const S &format,
basic_format_args<typename buffer_context<Char>::type> args) {
if (tf.has_emphasis()) {
if (ts.has_emphasis()) {
internal::fputs<Char>(
internal::make_emphasis<Char>(tf.get_emphasis()), stdout);
internal::make_emphasis<Char>(ts.get_emphasis()), f);
}
if (tf.has_foreground()) {
if (ts.has_foreground()) {
internal::fputs<Char>(
internal::make_foreground_color<Char>(tf.get_foreground()), stdout);
internal::make_foreground_color<Char>(ts.get_foreground()), f);
}
if (tf.has_background()) {
if (ts.has_background()) {
internal::fputs<Char>(
internal::make_background_color<Char>(tf.get_background()), stdout);
internal::make_background_color<Char>(ts.get_background()), f);
}
vprint(format, args);
internal::reset_color<Char>(stdout);
vprint(f, format, args);
internal::reset_color<Char>(f);
}

/**
Formats a string and prints it to stdout using ANSI escape sequences to
specify text formatting.
Formats a string and prints it to the specified file stream using ANSI
escape sequences to specify text formatting.
Example:
fmt::print(fmt::emphasis::bold | fg(fmt::color::red),
"Elapsed time: {0:.2f} seconds", 1.23);
*/
template <typename String, typename... Args>
typename std::enable_if<internal::is_string<String>::value>::type
print(const text_style &tf, const String &format_str, const Args & ... args) {
typename std::enable_if<internal::is_string<String>::value>::type print(
std::FILE *f, const text_style &ts, const String &format_str,
const Args &... args) {
internal::check_format_string<Args...>(format_str);
typedef typename internal::char_t<String>::type char_t;
typedef typename buffer_context<char_t>::type context_t;
format_arg_store<context_t, Args...> as{args...};
vprint(tf, format_str, basic_format_args<context_t>(as));
vprint(f, ts, format_str, basic_format_args<context_t>(as));
}

/**
Formats a string and prints it to stdout using ANSI escape sequences to
specify text formatting.
Example:
fmt::print(fmt::emphasis::bold | fg(fmt::color::red),
"Elapsed time: {0:.2f} seconds", 1.23);
*/
template <typename String, typename... Args>
typename std::enable_if<internal::is_string<String>::value>::type print(
const text_style &ts, const String &format_str,
const Args &... args) {
return print(stdout, ts, format_str, args...);
}

#endif
Expand Down
1 change: 1 addition & 0 deletions include/fmt/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,7 @@ namespace internal {

struct dummy_string_view { typedef void char_type; };
dummy_string_view to_string_view(...);
dummy_string_view to_string_view(const std::FILE *);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do fmt::print(stderr, fmt::emphasis::bold, "");, OR kicks in. It has a lot of candidates:

print(std::FILE *, const text_style &, format_str, args...); // this is what we want to get called
print(format_str, args...);

Normally, one would expect the first one to get called, but that's not the case. This is due to variadic templates being greedy. fmt::emphasis::bold is not a text_style and since stderr is a pointer to T, the second overload above is taken, which then fails with an assertion failure because std::FILE is not a valid character type.

I'm adding this so that fmt doesn't think that stderr is a valid format string - This never was an issue before because there were always exact matches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be much nicer to disregard all pointer types cv T * for which no specialization std::char_traits<T> exists? May be by detecting the existence of the std::char_traits<T>::char_type type member which requires only type-SFINAE and should work on all compilers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DanielaE Didn't want to change this for such a patch, but yeah that's the better solution. I'll update my PR then :) thanks

Copy link
Contributor

@DanielaE DanielaE Dec 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In this case I'd rather split the PR into two: the first one laying the cv T * groundwork and then the second one actually addressing issue #944 taking advantage of the first PR. This looks cleaner to me but at the end of the day it's up to Victor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it looks like std::char_traits is defined for everything including FILE =), so we'll have to rely on some mechanism within {fmt} to detect char types. to_string_view is indeed our current way to detect string types and therefore character types, so unless there are better ideas let's go with the current to_string_view-based solution but please add a comment briefly explaining what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

😢 This sounds like a language or library defect. Characters are fundamental entities in the language and there are predicates to detect almost any kind of trait, but a well-defined predicate to detect the "character-ness" type trait seems to be missing. I've sifted through MSVC's library and found the catch-all primary template of std::char_traits which defines the char_type member to T. Interestingly enough, only in this non-character case the int_type is defined to long. But this cannot be relied on because the specification in the standard is weasel-wordy in the case of non-required specializations.

Copy link
Contributor

@vitaut vitaut Dec 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

One more thing: to_string_view(const std::FILE *) should probably go to fmt/color.h since it is only relevant for the print overloads there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vitaut It doesn't seem to work when I move the overload to color.h - don't know why. So I changed it to two explicit specializations instead. Hope that's ok too :)

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 specializations are even clearer.

using fmt::v5::to_string_view;

// Specifies whether S is a string type convertible to fmt::basic_string_view.
Expand Down
4 changes: 4 additions & 0 deletions test/format-impl-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,4 +228,8 @@ TEST(ColorsTest, Colors) {
stdout,
fmt::print(fg(fmt::color::blue) | fmt::emphasis::bold, "blue/bold"),
"\x1b[1m\x1b[38;2;000;000;255mblue/bold\x1b[0m");
EXPECT_WRITE(stderr, fmt::print(stderr, fmt::emphasis::bold, "bold error"),
"\x1b[1mbold error\x1b[0m");
EXPECT_WRITE(stderr, fmt::print(stderr, fg(fmt::color::blue), "blue log"),
"\x1b[38;2;000;000;255mblue log\x1b[0m");
}