-
Notifications
You must be signed in to change notification settings - Fork 85
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 basic <format>
formatter (C++20)
#70
base: master
Are you sure you want to change the base?
Conversation
I forgot to include EDIT: Fixed, hopefully |
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 @AbitTheGray, sorry for the delay in the review!
I appreciate your contribution, it's certainly something I'd like to support in principle, but I did have a few comments that would require some significant effort to fix. I hope you might be willing and able to make this happen.
include/fpm/ios.hpp
Outdated
@@ -737,4 +737,44 @@ std::basic_istream<CharT, Traits>& operator>>(std::basic_istream<CharT, Traits>& | |||
|
|||
} | |||
|
|||
#if __cplusplus >= 202002L | |||
# include <version> | |||
#endif |
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.
I think this #endif
should be moved to the end, because as you said, __cpp_lib_format
won't work if we don't include <version>
.
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.
In theory you could have the support by defining __cpp_lib_format
but let's consider it non-standard. So yes, I'll use this #if
as a wrapper around the whole code block.
include/fpm/ios.hpp
Outdated
out << std::showpos; | ||
out << value; | ||
|
||
return std::ranges::copy(std::move(out).str(), ctx.out()).out; |
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.
Since this doesn't assume that std::format
exists (as per the version check), should this assume that std::ranges
exists? Perhaps a more standard std::copy
is preferable here.
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.
This is inside #ifdef __cpp_lib_format
but you are right about std::ranges
, I'll just use standard copy 👍
include/fpm/ios.hpp
Outdated
auto it = ctx.begin(); | ||
if(it != ctx.end() && *it == '+') | ||
{ | ||
showpos = true; |
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.
I think if this now exposes std::format
capabilities, it should be done completely and fully. Would you be able to add support for the full float formatting specification? https://en.cppreference.com/w/cpp/utility/format/spec
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.
I will do my best but I've never wrote this complex formatting. I just hope I'll be able to put the output together with existing operator<<
(that is a complex one).
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.
Could you please add sufficient tests for this new functionality?
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.
Sure. Now that I know you are OK with including it (and want full "Standard format specification"), it is definitely good idea to add tests. With the first implementation, I just tried few variants. (I know, it is not the way to test it)
Nested width/precision is not supported `wchar_t` tests are copy of existing ones with different type Hex test is disabled as `fpm` implementation differs from `double` one
I think that I've implemented everything I can. Sadly there are 2 missing features:
I was hoping the nested width and precision is auto-solved before being handed to the For the hex one, value of It is possible there are still some bugs as I've written only a small subset of possible tests. I've focused only on correct, well defined behavior. |
TD;DR: Enable
std::format
forfpm::fixed
with minimum code.I have been using
std::format
from<format>
(C++20) as it does not require you to specify the type, which I consider big upgrade fromprintf
. While fpm has stream operators inios.hpp
, you need to specializestd::formatter<,>
to use your own type. I've implemented very simple one just to allow me to print it anywhere - the only option there is+
, as{0:+}
for first argument, to always include sign character (usingstd::showpos
). It usesstd::stringstream
to redirect the existing<<
operator.I understand that the
std::format
probably has more overhead thanprintf
which may not sit well with people who want to use fixed-point numbers due to them being faster than floating-point numbers. But you are not required to ever callstd::format
if that is a problem (or not supported by your platform) and I consider thestd::format
quality-of-life addition.The specialized formatter is behind C++ feature test (
__cpp_lib_format
without version check as thestd::formatter
is there from the beginning, there is also__cpp_lib_formatters
which is for C++23 additions).Possible enhancements: