returning a string from a function

Question

I wanted to write a function that'll be cross platform (win32 & linux), and return a string representation of the datetime [hh:mm:ss dd-mm-yyyy].

Knowing that I just want to use the returned string as a temporary in a stream fashion as below:

std::cout << DateTime() << std::endl;

I considered writing a function with the following prototype

const char* DateTime();

If you return a character array, you must delete it once you're done. But I just want a temporary, I don't want to have to worry about de-allocating the string.

So I've written a function that just returns an std::string:

#include <ctime>
#include <string>
#include <sstream>

std::string DateTime()
{
    using namespace std;

    stringstream ss;
    string sValue;
    time_t t = time(0);
    struct tm * now = localtime(&t);

    ss << now->tm_hour << ":";
    ss << now->tm_min << ":";
    ss << now->tm_sec << " ";
    ss << now->tm_mday + 1 << " ";
    ss << now->tm_mon + 1 << " ";
    ss << now->tm_year + 1900;

    sValue = ss.str();

    return sValue;
}

I realize that I'm returning a copy of the stack variable in DateTime. This is inefficient in that we create the string on the DateTime stack, populate it, then return a copy and destroy the copy on the stack.

Has the c++11 move-semantics revolution done anything to resolve this inefficiency - can I improve upon this?

1) NRVO makes this a complete non-issue. 2) If NRVO _didn't_ kick in for whatever reason, then yes, the return value would be moved rather than copied. — ildjarn, Jun 29 '12 at 22:45
@lapin: named return value optimization. Instead of copying a value (perhaps multiple times) to get from the called function back to the caller, the compiler passes the function a hidden pointer to the location where it will end up being assigned, and the function uses that location for its local variable. — Jerry Coffin, Jun 29 '12 at 22:49
like other have said RVO is guaranteed if you are using C++11, however I wouldn't worry because the string stream will be the bottle neck here. — 111111, Jun 29 '12 at 23:39
@111111 : RVO is never guaranteed in the standard (98, 03, or 11), merely permitted. It's up to the implementation to make guarantees regarding this particular optimization. — ildjarn, Jun 30 '12 at 01:09
`tm_mday + 1` - you should remove this `-1`, `tm_mday` is 1-based. — Alexey Frunze, Jun 30 '12 at 09:57

score 6 · Accepted Answer · answered Jun 30 '12 at 01:12

lapin, your code is fine C++11 code. In C++98/03 your code will probably be efficient due to compiler optimizations, but those optimizations aren't guaranteed. In C++11, those same optimizations will probably still make your return free, but just in case they don't, your string will be moved instead of copied.

So return by value guilt-free! :-)

Minor nit:

It is best practice to declare your values at the point of first use, instead of at the top of a block:

string sValue = ss.str();
return sValue;

Or perhaps even:

return ss.str();

But this is just a minor nit. Your code is fine and efficient.

I started the (bad?) habit of declaring my variables before my code when I was writing far too many MSSQL stored procedures a few years ago. I've heard your point before on declaring vars at the point of first use for efficiency, and I agree with it, but I still find myself declaring before the code :) — fishfood, Jun 30 '12 at 20:01

Nevin · Answer 2 · 2012-06-30T08:42:42.317

Another way to do this is to make it a function object with a stream inserter, as in:

struct DateTime()
{
    friend std::ostream& operator<<(std::ostream& os, DateTime)
    {
        time_t t = time(0);
        struct tm * now = localtime(&t);

        os << now->tm_hour << ":";
        os << now->tm_min << ":";
        os << now->tm_sec << " ";
        os << now->tm_mday + 1 << " ";
        os << now->tm_mon + 1 << " ";
        os << now->tm_year + 1900;

        return os;
    }

    // Could be converted to a static method,
    //  since DateTime has no internal state
    std::string str() const
    {
        // the following 3 lines can be replaced by
        //  return boost::lexical_cast<std::string>(*this);
        std::ostringstream ss;
        ss << *this;
        return ss.str();
    }

    operator std::string() const
    { return str(); }
};

Pete Fordham · Answer 3 · 2012-06-30T02:15:10.300

In a world without RVO/NRVO this should avoid the copy construction in a pre C++11 standard library. In a post C++11 library with move constructor for string it still avoids the move constructor being called; it's probably a trivially small difference but still the OP was asking how to do better.

(And yes I agree the inheriting from string is ugly but it does work.)

#include <ctime>
#include <string>
#include <sstream>
#include <iostream>

using namespace std;

class DateString : public string {

public:
DateString() : string()     {

    stringstream ss;
    time_t t = time(0);
    struct tm * now = localtime(&t);

    ss << now->tm_hour << ":";
    ss << now->tm_min << ":";
    ss << now->tm_sec << " ";
    ss << now->tm_mday + 1 << " ";
    ss << now->tm_mon + 1 << " ";
    ss << now->tm_year + 1900;

    append(ss.str());

}
};

int main()
{
    cout << DateString() << endl;
    return 0;
}

1) Inheriting from `std::string` is just (morally) wrong. 2) The code the OP already has will also avoid copy construction even without NRVO. — ildjarn, Jun 29 '12 at 23:57

score -1 · Answer 4 · answered Jun 30 '12 at 01:16

-1

Ok, I know this is not thread safe and all that and I'll probably be downvoted to hell end back, but I've seen the following in a library that I'm using (CERN's ROOT):

const char * myfunc(){

  static std::string mystr;

  /*populate mystr */

  return mystr.c_str();
}

This only works if you know that no one will ever be so stupid as to hold on to the pointer.

This is a way of having a temporary that will not leak no matter what.

answered Jun 30 '12 at 01:16

Simon

961
2
9
14

That was fast DeadMG! :-) The problem with this answer is that it invokes undefined behavior. By the time client sends the pointer to `cout`, the local `mystr` has already deleted the character array to which that pointer points to. It might work, and I have no doubt Simon tested before posting. But it also might not work. In a multithreaded environment the same application might sometimes work and sometimes not. – Howard Hinnant Jun 30 '12 at 01:22
3

@Howard : `mystr` is static, so there's no destruction of it here; I think DeadMG's nit is that this isn't thread-safe. – ildjarn Jun 30 '12 at 01:35
@ildjarn: So it is! Thanks for the correction. I completely overlooked that. What's the convention here? Delete my comment because it might confuse people, or leave it up so they can read this one too? – Howard Hinnant Jun 30 '12 at 01:37
@Howard : Your call; it might confuse people, or it might be clarifying for other readers that make the same mistake you did when reading the code. :-] – ildjarn Jun 30 '12 at 01:38

returning a string from a function

4 Answers4

Linked