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

Consistent behavior of operator<< for known types #495

Closed
wants to merge 5 commits into from

Conversation

effzeh
Copy link
Contributor

@effzeh effzeh commented Apr 11, 2017

Consistent behavior of operator<< for known types.

The (templated) operator<< defined in ostream.h currently prevents efficient
handling of known types in BasicWriter. E.g.:

  MemoryWriter w;
  w << std::string("hello");

calls this operator (if visible). This will unneccessarily construct a
std::ostream object using a FormatBuf as its streambuf and insert the string
into the std::ostream object. (The same is true for float and some other
built-in types.) The write_str method in BasicWriter basically does the same
using a cheap memcpy.

This patch attempts to fix this performance problem.

But this patch might also break some code, e.g.:

    std::string s = (MemoryWriter() << 3.14).str();

only works in C++11 now. Note that

    std::string s = (MemoryWriter() << UserDefinedType()).str();

didn't work before, but should work now.

I haven't found another solution which works and is still safe (neither in C++03
nor in C++11), so I'm just posting this here for discussion...

@vitaut
Copy link
Contributor

vitaut commented Apr 15, 2017

Thanks a lot for the PR!

But this patch might also break some code, e.g.:
std::string s = (MemoryWriter() << 3.14).str();

This is a bit unfortunate. I'll try to come up with an alternative solution that doesn't break existing code. If not then I guess we'll have to go with the backward-incompatible one.

effzeh added 3 commits May 6, 2017 14:01
The (templated) operator<< defined in ostream.h currently prevents efficient
handling of some basic types in BasicWriter. E.g.:

    MemoryWriter w;
    w << std::string("hello");

calls this operator (if visible). This will unneccessarily construct a
std::ostream object using a FormatBuf as its streambuf and insert the string
into the std::ostream object. (The same is true for float and some other
built-in types.) The write_str method in BasicWriter basically does the same
using a cheap memcpy.

Fix this performance problem by providing additional overloads of operator<<.
For MSVC this was a better than the rvalue->lvalue conversion
@effzeh
Copy link
Contributor Author

effzeh commented May 6, 2017

Should be backwards compatible now. Turned out the only difficulty were some inconsistences between MSVC and other compilers. If this gets accepted, the commits should probably be squashed... :-)

vitaut added a commit that referenced this pull request May 6, 2017
Revert #456 because it causes issues for known types (#495) and is not C++98-compatible.
@vitaut
Copy link
Contributor

vitaut commented May 6, 2017

Thanks for working on this. Unfortunately there is still an issue with implicit conversions, so I decided to revert (dcfd40a) the commit that introduced this problem in the first place because it not only interferes with known types but is also not compatible with older compilers which is one of the goals of this library. operator<< is likely to go away eventually in favor of a more expressive API based on named arguments: #462.

@vitaut vitaut closed this May 6, 2017
@effzeh
Copy link
Contributor Author

effzeh commented May 6, 2017

Yes, thats clearly the easiest solution. Thanks!

bjoernthiel pushed a commit to AbberiorInstruments/fmt that referenced this pull request May 11, 2017
Revert fmtlib#456 because it causes issues for known types (fmtlib#495) and is not C++98-compatible.
@effzeh effzeh deleted the lessless branch June 10, 2017 14:36
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