Formatter class

Question

In our production code, we cannot use Boost or C++0x. Formatting strings using sprintf or stringstream is annoying in this case, and this prompted me to write my own little Formatter class. I am curious if the implementation of this class or the use of it introduces any Undefined Behavior.

In particular, is this line fully-defined:

Reject( Formatter() << "Error Recieved" << 42 << " " << some_code << " '" << some_msg << "'");

My belief is that it is OK, but I wanted peer-review.

Three main points of concern:

1. Is there a double-assignment within a single sequence point? Is it UB?
2. Do I have a problem with the lifetime of temporaries?
3. Does my Formatter class (or the intended use of it) introduce any UB?

The Formatter class has both a (templatized) operator<< and an operator std::string. The intent is to use the Formatter() class as a temporary in place of a std::string parameter for any function taking a const std::string&.

Here is the class definition:

class Formatter
{
public:
 Formatter() {};
 template<class Field> Formatter& operator<<(Field f)
 {
  ss_ << f;
  return *this;
 }
 operator std::string() const { return ss_.str(); }
private:
 std::stringstream ss_;
};

And here is a complete test harness, including the above definition. You should be able to compile & run as-is. Do you see any UB?

#include <cstdlib>
#include <string>
#include <sstream>
#include <iostream>

class Formatter
{
public:
 Formatter() {};
 template<class Field> Formatter& operator<<(Field f)
 {
  ss_ << f;
  return *this;
 }
 operator std::string() const { return ss_.str(); }
private:
 std::stringstream ss_;
};

void Reject(const std::string& msg)
{
 std::cout << "Recieved Message: '" << msg << "'" << std::endl;
}

int main()
{
 const char& some_code = 'A';
 const char* some_msg = "Something";

 Reject( Formatter() << "Error Recieved" << 42 << " " << some_code << " '" << some_msg << "'");
}

Answer 1

In addition to what's already been said, I would:

• Mark the stringstream as public. This won't affect most uses of your code, and can already be hacked around with a custom manipulator to get at the "internal" stream object, but it will enable those that need to access the internal stream (such as to avoid the string copy inherent in the stringstream interface), and know the specifics of their implementation that allow what they want, to do so. Of course, 0x move semantics allay much of this need, but are still Not Quite Here Yet™.

• Check the stream before returning the string; if it's in a failed state, throw an exception (or at least log the condition somewhere before returning a string). This is unlikely to occur for most uses, but if it does happen, you'll be glad you found out the stream is failed rather than screw with formatting while wondering why "it just won't work".

Regarding double-assignment, there's no assignment at all. The sequence points should be mostly what people expect, but, exactly, it looks like:

some_function(((Formatter() << expr_a) << expr_b) << expr_c);
//                          1          2          3

The operators order it as if it was function calls, so that:

• Formatter() and expr_a both occur before the insertion marked 1.
• The above, plus insertion 1, plus expr_b happen before insertion 2.
• The above, plus insertion 2, plus expr_c happen before insertion 3.
• Note this only limits in one direction: expr_c can happen after expr_a and before Formatter(), for example.
• Naturally, all of the above plus the string conversion occur before calling some_function.

To add to the discussion on temporaries, all of the temporaries created are in the expression:

some_function(Formatter() << make_a_temp() << "etc.")
//            one temp       another temp     and so on

They will not be destroyed until the end of the full expression containing that some_function call, which means not only will the string be passed to some_function, but some_function will have already returned by that time. (Or an exception will be thrown and they will be destroyed while unwinding, etc.)

In order to handle all manipulators, such as std::endl, add:

struct Formatter {
  Formatter& operator<<(std::ios_base& (*manip)(std::ios_base&)) {
    ss_ << manip;
    return *this;
  }
  Formatter& operator<<(std::ios& (*manip)(std::ios&)) {
    ss_ << manip;
    return *this;
  }
  Formatter& operator<<(std::ostream& (*manip)(std::ostream&)) {
    ss_ << manip;
    return *this;
  }
};

I've used this pattern several times to wrap streams, and it's very handy in that you can use it inline (as you do) or create a Formatter variable for more complex manipulation (think a loop inserting into the stream based on a condition, etc.). Though the latter case is only important when the wrapper does more than you have it do here. :)

Answer 2

Just three comments:

1. I would remove the empty constructor.

2. What about handling std::manipulators?

3. Don't you want to pass field values by const reference?

.

 template<class Field> Formatter& operator<<(Field const& f)
                                                // ^^^^^^

Your concerns:

1. Is there a double-assignment within a single sequence point? Is it UB?

Don't see one.
Looks good.

1. Do I have a problem with the lifetime of temporaries?

No. Don't think so.

Answer 3

I think you might be worried about temporary lifetime issues. (I know I have based on similar code.)

The temporary object created as a parameter for Reject will have a lifetime bound to the expression it is created in. (It's in the C++ standard somewhere.) So even with your conversion operators returning values, they will all be destructed after the expression that contains Reject.

Answer 4

• I'd use a std::ostringstream because you don't use (seem to need) the formatting capabilities of std::istringstream.
• What happens when formatting fails? Can you check for failure?

Answer 5

Looks ok. The empty ctor is unnecessary; the compiler generated one will do just fine. "Recieved" should be spelled "Received" :)

Answer 6

Looks fine. Don't worry too much and end up over-engineering a simple solution :)

Edit:

Actually, I would make the parameters to operator<< into const references.