Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've been thinking of using this:

template <typename T>
inline T from_string(const std::string& s){
   std::istringstream iss;
   iss.str(s);
   T result;
   iss >> result;
   return result;
}

in my code. Then I thought "I shouldn't construct istringstreams all the time", and made it into this:

inline std::istringstream& get_istringstream(){
    static thread_local std::istringstream stream;
    stream.str("");
    return stream;
}

template <typename T>
inline T from_string(const std::string& s){
   auto& iss(get_istringstream());
   iss.str(s);
   T result;
   iss >> result;
   return result;
}

... and this builds and works (although I haven't tested it very extensively, nor ran performance tests). Would you say that "good enough" for general-purpose utility code, that is not intended to run in some tight loop? Are there other considerations I've overlooked, performance-wise (*) or usability-wise?

Perhaps I should mention my motivation here is partly how I found it strange that there's no std::from_string().

Edit: If you're concerned about the dependence on a default constructor, we can also throw in this:

template< typename T >
struct istream_traits {
    inline static T read(std::istream& is)
    {
        T x;
        is >> x;
        return x;
    }
};

template<> struct istream_traits<bool> {
    inline static bool read(std::istream& is)
    {
        is >> std::boolalpha;
        bool x;
        is >> x;
        return x;
    }
};

template<typename T>
inline T read(std::istream& is)
{
    T x = istream_traits<T>::read(is);
    return x;
}

... and then replace T result; iss >> result; with return read<T>(iss);.


(*) - Yes, I know anything with iostreams is probably not fast to begin with.

share|improve this question
5  
Does it work? Do you like how it works? What are your requirements? We can't say whether it's 'good enough' if you don't tell us what it's being used for. – Mast Aug 26 '16 at 14:52
    
@Mast: See edit. I think I like how it works. It's going into my project's "general utility code"; my immediate use is a bit of configuration parsing. – einpoklum Aug 26 '16 at 14:59
    
@OlzhasZhumabek: Using a stringstream, you would need to: 1. have 3 instead of 1 statements and 2. always remember to use a single thread-local stringstream rather than creating and destroying one, which IIRC has some overhead. – einpoklum Aug 26 '16 at 15:22
    
@OlzhasZhumabek: Oh, I have an extra complication for that... see Edit. – einpoklum Aug 26 '16 at 15:37
1  
@einpoklum: It extends to arbitrary types in much the same way yours does: if you can extract that type from a stream, it works. Big differences are that it also supports input types other than string, has specializations to speed it up for common types, and checks that the conversion was successful at the end (i.e., that the stream extraction exhausted the content of the stream). – Jerry Coffin Aug 26 '16 at 21:59

Just going to review your original code and not the edit.

inline std::istringstream& get_istringstream(){
    static thread_local std::istringstream stream;
    stream.str("");
    return stream;
}

template <typename T>
inline T from_string(const std::string& s){
    auto& iss(get_istringstream());
    iss.str(s);
    T result;
    iss >> result;
    return result;
}

As Jerry Coffin mentioned, you are basically reinventing boost::lexical_cast. I wasn't sure if I wanted to review this or flag it as broken code, but it technically compiles and it technically works for one conversion (perhaps a discussion for meta-cr). Test your code thoroughly.


Are there other considerations I've overlooked, performance-wise (*) or usability-wise?

You don't handle errors. Consider someone doing this:

auto x1 = from_string<int>("Not an int");

What is the value of x1?

    T result;      // uninitialized variable.
    iss >> result; // operator>> fails silently, sets failbit
    return result; // return uninitialized variable.

The result is undefined behavior since we are returning an uninitialized value that was never assigned to. Remember to check that the reading of a value was successful. Notify the callee whenever that an error, like invalid argument or out of range, has occurred.

There are different error handling strategies today. The common ones are exceptions, std::system_error, and Alexandrescu's expected<T, E> (currently being proposed for standardization).


Let's say someone does something common, like use your function more than once.

auto x1 = from_string<int>("42");
auto x2 = from_string<int>("43");

The first line returns 42, that is fine. The second line has an issue.

    T result;      // uninitialized variable.
    iss >> result; // eofbit enabled from previous stream consumption
    return result; // return uninitialized variable.

Undefined behavior again. The original code had

std::istringstream iss;

On construction, isss internals were initialized (data and error state). In your refactored code, you wanted to mimic a newly constructed object

inline std::istringstream& get_istringstream(){
    static thread_local std::istringstream stream;
    stream.str("");
    return stream;
}

    auto& iss(get_istringstream());

Turns out your refactored code wasn't equivalent. You forgot to clear the error state.


Consider the following code:

auto x1 = from_string<double>("42,004");

What is the value of x1? Conversion of arithmetic types using separators is dependent on the locale of the stream. The possible results are

  • forty-two thousand and four,
  • forty two and four one-thousandths, or
  • forty-two.

If you are fine with this behaviour, document it. If you want to ensure you always use the behavior of a specific locale by default, consider imbuing before reading.


Perhaps I should mention my motivation here is partly how I found it strange that there's no std::from_string().

<std::string> provides functions to convert to arithmetic types

You could also consider copying the naming scheme from these and have string_to<T>.

For general purpose conversions, the ones that immediately come to mind are boost::lexical_cast (stream-based) and boost::spirit (policy-based). Boost also has boost::convert, an adapter library built to work with various conversion utilities (strtol, boost::spirit, boost::lexical_cast, and C++-Streams).

share|improve this answer
    
That's awesome. I feel you've gone to way too much effort on my account... and while I know about std::stowhatever, I did not know about boost::lexical_cast before I asked this question, and now I also know why it won't do (default constructibility) for my uses, and you've introduced me to boost::convert which I need to study. – einpoklum Aug 27 '16 at 8:02
    
Also, are you sure an istringstream's eof bit stays set even when you str() it? – einpoklum Aug 27 '16 at 8:02
    
Yes. It's also not just the eofbit, but also badbit and failbit. – Snowhawk04 Aug 27 '16 at 8:54
    
I would have believed you if you just said "Yes I'm sure" :-) ... anyway, that's unintuitive, but what'll you do. Ok. – einpoklum Aug 27 '16 at 8:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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