25
\$\begingroup\$

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 << "'");
}
\$\endgroup\$
9
\$\begingroup\$

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. :)

\$\endgroup\$
  • \$\begingroup\$ The danger is that expr_c can happen at the same time as expr_a. \$\endgroup\$ – Deduplicator Oct 20 '15 at 11:49
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ What's the problem with manipulators? You can still use the templated insertion operator for them. Formatter() << std::hex;. Do you mean stuff like Formatter().width(10) ? \$\endgroup\$ – wilhelmtell Jan 27 '11 at 3:43
  • \$\begingroup\$ @wilhelmtell: Actually I was thinking about std::endl. But that may be by design. Though the use of std::endl may not have much meaning in context with the Formatter object it may still confuse some users that are simply porting old code that this streamable object will not compile when used with std::endl. \$\endgroup\$ – Martin York Jan 27 '11 at 5:24
  • \$\begingroup\$ Manipulators will not work directly. You need to manually add support for them. Look at the answer by Fred Nurk \$\endgroup\$ – David Rodríguez - dribeas Jan 27 '11 at 15:25
  • \$\begingroup\$ @David Rodríguez - dribeas: Which is exactly why I mentioned it. \$\endgroup\$ – Martin York Jan 27 '11 at 19:40
  • \$\begingroup\$ York, I should have added some @wilhelmtell there. The comment was not about your answer, but rather about his comment. Sorry for the confusion :) \$\endgroup\$ – David Rodríguez - dribeas Feb 2 '11 at 12:15
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ +1: I was also concerned about double-assignment within a single sequence point. \$\endgroup\$ – John Dibling Jan 26 '11 at 22:55
5
\$\begingroup\$
  • 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?
\$\endgroup\$
4
\$\begingroup\$

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

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I agree! I was hoping there was no problem, because Formatter is a very small gizmo, indeed. \$\endgroup\$ – John Dibling Jan 26 '11 at 22:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.