-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Adapt any string-like type to be used by {fmt} just like the standard… #907
Conversation
This feature enables using string types like Qt's QString/QStringRef/QStringView, MFC/ATL's CString, Xerces' XStr, and many more. Any strings or string-like types like views with stable references can be adapted to yield compatible fmt::basic_string_views. The adaption neither intrudes fmt::basic_string_view nor the adapted string type. @vitaut Victor, what do you think about it? |
Interesting. Do you envision this to be used primarily for formatting arguments or format strings as well? If the former then it is already possible to support arbitrary string types via the extension API: template <>
struct fmt::formatter<MyString> : fmt::formatter<string_view> {
auto format(const MyString& s, format_context& ctx) {
return formatter<string_view>::format({s.data(), s.size()}, ctx);
}
}; Edit: fixed example. |
I see the use case in both the format string and the formatting arguments. I am particularily interested in using At present I am still working on exploring the ease (or tediousness) of using the extension API of |
Sounds reasonable. One question regarding the API: is there any particular reason to use specialization and not function overloading, possibly with ADL? Something along the lines of template <typename Char>
fmt::basic_string_view<Char> to_string_view(const StringType<Char> &S) {
return { S.data(), S.length() };
}
// and then enable_if on to_string_view(declval<StringType<Char>>()
That's awesome, thank you for doing this! Let me know if you have any ideas on improving the extension API.
I don't mind at all and will be happy to answer any question you might have. |
I have multiple reasons for that decision:
|
There were multiple suggestions to replace specialization with overloading there as well, most recently in #518 (comment), so I wouldn't necessarily recommend trying to mimic it.
I think the character type can also be detected in the overload case from the return type of
The current approach is to try disallowing conversions that result in temporaries whenever possible.
The only reason it lives in the
AFAICS ADL works with inline namespaces too: https://godbolt.org/z/k5QOnP All that said, I don't feel strongly about it and see good arguments both ways. |
Right. At the end of the day, you are the master chief of {fmt}, so you decide about the direction of development. 😄 My strong expectation as user of a library is as much ease-of-use as reasonably possible to implement without putting too much burden on the developers of the library. In our team we prefer less typing over more typing (the latter usually a result of the notion of making anything explicit that can be made explicit) wherever possible without introducing nasty sideeffects. Therefore I prefer solutions being welcoming to as many string-like types as possible to be digested by {fmt} without requiring additional hoops by users when they call call fmt::functions. |
ADL-based approach seems slightly simpler so I'd recommend going with it unless there are some technical obstacles to applying it here. (For example, in the extension API the |
85aa256
to
b50e7ec
Compare
As it turned out, using ADL detection given the unlimited zoo of class types rather than partially specializing a well-known template class is incredibly difficult to implement (at least with my limited capabilities in metaprogramming). My very first attempt worked very well with VS2017, VS2015, the clang versions from your Travis setup, and gcc 8.1 in my mingw environment. All other compilers (i.e. VS2013 and gcc at least up to version 6) were failing like crazy. I suspect that they all suffer from the same problem (at least the failures being identical suggest that): they go a bridge to far with accepting conversions during overload resolution. After days of despair I finally came up with some kind of hack to prevent these old compilers from even attempting the search for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! Just one comment re to_string_view
namespace. Sorry you had to go through all the hoops to implement the ADL-based approach. I thought it would be easier but, as it is often the case, legacy compilers are a pain. I'll see if we can simplify this a bit, but that's not critical; the most important thing is that the user code looks simple.
include/fmt/core.h
Outdated
std::string message = fmt::format(StringType<char>("The answer is {}"), 42); | ||
\endrst | ||
*/ | ||
|
||
template <typename S> | ||
inline basic_string_view<FMT_CHAR(S)> to_string_view(const S &s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that to_string_view
is an extension point, it should live in the fmt
namespace. This will also eliminate the need for using internal::to_string_view
and, likely, the internal2
namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Victor!
Actually, the fact that the built-in to_string_view
sits in a sibling namespace to internal2
is the gist of the magic that makes the whole kaboodle work in the first place! In fact, the ADL detection would work much easier without all of this crazy stuff if the detection class could sit in a sibling namespace of fmt
itself (which it can't). As soon as you move internal::to_string_view
and the internal2
detection logic into the same namespace (or the detection logic into a child namespace such as internal
), the ADL search would pick up the built-in to_string_view
as possibly viable candidate again if a compiler finds some acceptable conversion path. The whole point of the ADL logic is to find only as exact matches as possible without competing overloads.
Did you happen to run a quick test on your machine with your suggested changes? In particular with some of the mentioned problematic compilers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've run a quick check on my Visual Studio here. Removing all occurences of using internal::to_string_view' and relocation
to_string_viewfrom namespace
fmt::internalto namespace
fmt` made no difference on VS2017, but VS2013 (and possibly all of the old gcc versions) is failing again (in this case when compiling format.cc) :
1>------ Build started: Project: fmt, Configuration: Debug x64 ------
1>format.cc
1>E:\fmt\vs\fmt/format-inl.h(236): error C2783: 'std::basic_string<Char,std::char_traits<Char>,std::allocator<_Other>> fmt::v5::vformat(const S &,fmt::v5::basic_format_args<buffer_context<Char>::type>)' : could not deduce template argument for 'Char'
1> e:\fmt\vs\fmt\core.h(1498) : see declaration of 'fmt::v5::vformat'
1>E:\fmt\vs\fmt/format-inl.h(236): error C2660: 'fmt::v5::format_system_error' : function does not take 2 arguments
1>E:\fmt\vs\fmt/format-inl.h(910): error C2752: 'fmt::v5::internal::format_string_traits<fmt::v5::wstring_view,void>' : more than one partial specialization matches the template argument list
1> e:\fmt\vs\fmt\core.h(511): could be 'fmt::v5::internal::format_string_traits<S,std::enable_if<std::is_base_of<fmt::v5::basic_string_view<S::char_type>,S>::value,void>::type>'
1> e:\fmt\vs\fmt\core.h(562): or 'fmt::v5::internal::format_string_traits<S,std::enable_if<fmt::v5::internal2::has_to_string_view<S>::value,void>::type>'
1>Done building project "fmt.vcxproj" -- FAILED.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even explicitly knocking-out fmt::basic_string_view
and derivatives by explicitly specializing accept_as_external_string<T>
to std::false_type
, and removing specializations of fmt::to_string_view
based on this predicate doesn't help.
It looks like my metaprogramming-fu isn't powerful enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you happen to run a quick test on your machine with your suggested changes? In particular with some of the mentioned problematic compilers?
Hmm, it does break the legacy compilers. There is also some redundancy between to_string_view
and format_string_traits
. Let me take a look in more details and report back.
This disambiguation is required for string-like types like |
7680883
to
5f8b1de
Compare
Test updated to catch implicit conversions. |
I've made |
c7c70aa
to
4a4cc8d
Compare
Do you prefer it this way or shall I squash those five commits into one? |
Definitely squashed. Also |
… string types already supported. The adaption is totally non-intrusive. Signed-off-by: Daniela Engert <[email protected]>
c7c70aa
to
d948e3a
Compare
It looks like the automated test are no longer triggered. I've run the tests for MSVC2013/2015/2017 here locally on my machine and triggered the Travis PR test manually from my GitHub repo: test results. All of them passed. |
Merged with minor tweaks (mostly stylistic to improve consistency with the rest of the project) in 2c81c85. Thanks for your work! |
Great - thanks a lot! |
It's basically Google style with snake_case (for standardization reasons) and exceptions: https://github.com/fmtlib/fmt/blob/master/CONTRIBUTING.rst . I should probably switch to clang-format as well. |
I'm a bit unclear from this thread on these two cases:
|
If you have a string type template <>
struct fmt::formatter<MyString> : fmt::formatter<string_view> {
auto format(const MyString& s, format_context& ctx) {
return formatter<string_view>::format({s.data(), s.size()}, ctx);
}
}; You don't need to use |
… string types already supported. The adaption is totally non-intrusive.
Signed-off-by: Daniela Engert [email protected]