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 file stream support for stylized text printing. #967

Merged
merged 4 commits into from
Dec 9, 2018

Conversation

Rakete1111
Copy link
Contributor

@Rakete1111 Rakete1111 commented Dec 6, 2018

This PR adds file stream support to color.h:

fmt::print(stderr, fmt::emphasis::bold, "bold error");

Fixes #944

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.

LGTM, but could you rebase and add a small unit test that exercises the new function? Thanks!

@@ -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

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

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.

include/fmt/color.h Outdated Show resolved Hide resolved
@Rakete1111 Rakete1111 reopened this Dec 7, 2018
@vitaut vitaut merged commit 7492760 into fmtlib:master Dec 9, 2018
@vitaut
Copy link
Contributor

vitaut commented Dec 9, 2018

Merged, thanks a lot for another great PR!

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