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 paddle::string::{F,S,}Printf #2661

Merged
merged 2 commits into from
Jun 29, 2017
Merged

Conversation

wangkuiyi
Copy link
Collaborator

@wangkuiyi wangkuiyi commented Jun 29, 2017

For example:

std::string s = string::Sprintf("%2.2f", 12.3456);
string::Printf("%2.2f", 12.3456);
string::Fprintf(std::cerr, "%2.2f", 12.3456);

This implementation depends on a simplifcation and port of tinyformat.

More detailed justification of this PR is in the file comment of paddle/string/printf.h:

// Compared with std::stringstream, there are primary purpose of
// string::Printf:
//
// 1. Type-safe printing, with why and how explained in
//    http://www.drdobbs.com/stringprintf-a-typesafe-printf-family-fo/184401999.
//    Implementation includes
//
//    https://github.com/c42f/tinyformat
//    boost::format
//    std::stringstream
//
//    std::stringstream is not convenient enough in many cases.  For example:
//
//      std::cout << std::setprecision(2) << std::fixed << 1.23456 << "\n";
//
//    boost::format is the most convenient one.  We can have
//
//      std::cout << format("%2% %1%") % 36 % 77;
//
//    or
//
//      format fmter("%2% %1%");
//      fmter % 36; fmter % 77;
//      std::cout << fmter.c_str();
//
//    But the overloading of % might be overkilling and it would be
//    more efficient if it can write to std::cout directly.
//
//    tinyformat has an interface compatible with the C-printf style,
//    and it can writes to a stream or returns a std::string:
//
//      std::cout << tfm::printf(
//                  "%s, %s %d, %.2d:%.2d\n",
//                  weekday, month, day, hour, min);
//
//    or
//
//      tfm::format(std::cout,
//                  "%s, %s %d, %.2d:%.2d\n",
//                  weekday, month, day, hour, min);
//
// 2. High-performance -- most printed strings are not too long and
//    doens't need dynamic memory allocation.  Many StringPrintf
//    implementations doesn't enforce type-safe, but are
//    high-performance, including
//
//    https://developers.google.com/optimization/reference/base/stringprintf/
//    https://github.com/adobe/chromium/blob/master/base/stringprintf.h
//    https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/stringprintf.h
// 
// According to
// https://github.com/c42f/tinyformat#compile-time-and-code-bloat,
// boost::format runs too slow and results in large executable binary
// files.  So here we port tinyformat.

While I was porting tinyformat.h, I removed support for old Mac OS X versions and some Windows macros. This saved many lines of source code. Let me know if we should add all these back.

Copy link
Member

@jacquesqiao jacquesqiao left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@reyoung reyoung left a comment

Choose a reason for hiding this comment

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

Amazing library for tinyformat. I will go to details in that library later, but paddle::string::printf.h looks good for me.

@reyoung
Copy link
Collaborator

reyoung commented Jun 29, 2017

Because this PR is needed by #2646, I just merge it to develop.

@reyoung reyoung merged commit b71843d into PaddlePaddle:develop Jun 29, 2017
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