Bad feeling about piece of code

This is a discussion on Bad feeling about piece of code within the C++ Programming forums, part of the General Programming Boards category; Hi I have been handled some code to update/maintain and there is this function that gets me very nervous. Code: ...

  1. #1
    Registered User
    Join Date
    Jun 2009
    Posts
    1

    Bad feeling about piece of code

    Hi

    I have been handled some code to update/maintain and there is this function that gets me very nervous.

    Code:
    const string ltos(const long val)
    	{
    	char buffer[41];
    	sprintf(buffer, "%ld", val);
    	return (const char *)buffer;
    	}
    Back in school, I was taught that returning a pointer to a local variable was a very bad thing to do. Here, it works, probably because the char* -> string implicit casting is made in the caller stack, so the pointer/string is still valid. But I'm getting some strange behaviours and I suspect that this function is at the core.

    First, what do you think about the code? And, if you think (as I do) that it is wrong, any suggestions on how to change it (without changing too much, it is a library and changing all the code that calls this function is not affordable?

    TIA

  2. #2
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    I suppose you could return a string explicitly, that'd be handy. I agree that you're probably okay in that you're returning a newly-constructed temporary const object by cast, I guess, but still. You could also, instead of char[] + sprintf, use stringstream + << and return the .str() object of that stringstream. (NOTE: I have no idea how much performance is an issue, but I'm not clear on the relative merits of converting char * to string vs. a stringstream.)

  3. #3
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,307
    That return statement is no different to:
    Code:
    	return string((const char *)buffer);
    So, as that shows, it is actually just fine.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by iMalc View Post
    That return statement is no different to:
    Code:
    	return string((const char *)buffer);
    So, as that shows, it is actually just fine.
    Of course, it would make it more obvious to write it this way than the originally posted code. And making things obvious always helps!

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #5
    The larch
    Join Date
    May 2006
    Posts
    3,573
    Even then, what's the point in the cast to const char*?
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  6. #6
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by anon View Post
    Even then, what's the point in the cast to const char*?
    It is probably not necessary in an explicit conversion. The compiler is unlikely to automatically convert a char [] into a string, but by casting, it finds the string(const char *) constructor, and is "happy" to compile the code. using explicit string(buffer) should work - I haven't tried it, but it's likely to work OK.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  7. #7
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,046
    In case you're interested, here's how you could do it using stringstreams:
    Code:
    #include <string>
    #include <sstream>
    
    std::string ltos(long number) {
        std::ostringstream stream;
        stream << number;
        return stream.str();
    }
    My favourite method, though, is to use something like this.
    Code:
    #ifndef CALLIS_MISC_STREAM_AS_STRING_H
    #define CALLIS_MISC_STREAM_AS_STRING_H
    
    #include <sstream>
    #include <string>
    
    namespace Callis {
    namespace Misc {
    
    /** A simple class which in effect that adds an insertion operator, <<, to the
        std::string class, as users of streams such as cout or stringstream expect.
    
        This class can be used as a temporary object in places where an ordinary
        string would be expected; for example:
            void print(std::string str);
            print(StreamAsString() << "Answer = " << 42);
    
        This implementation is quite efficient, only converting its internal
        stringstream to a string when operator std::string() is called.
    */
    class StreamAsString {
    private:
        /** The internal stringstream used to implement the insertion operator <<.
        */
        std::ostringstream stream;
    protected:
        std::ostringstream &get_stream() { return stream; }
        const std::ostringstream &get_stream() const { return stream; }
    public:
        /** Adds an object to the end of the internal stringstream.
        */
        template <typename T>
        StreamAsString &operator << (const T &data);
    
        /** Converts the internal stringstream to an std::string automatically.
        */
        operator std::string() const;
    };
    
    template <typename T>
    StreamAsString &StreamAsString::operator << (const T &data) {
        get_stream() << data;
    
        return *this;
    }
    
    inline StreamAsString::operator std::string() const {
        return get_stream().str();
    }
    
    } // namespace Misc
    } // namespace Callis
    
    #endif
    Then as the comment indicates you can use code like
    Code:
    long long_answer = 42;
    std::string answer = StreamAsString() << long_answer;
    and the StreamAsString object will automatically be converted to a std::string.
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 2
    Last Post: 05-14-2009, 04:13 PM
  2. Need a second opinion - code error and i cant see it
    By bigfootneedhelp in forum C Programming
    Replies: 19
    Last Post: 10-25-2007, 06:02 AM
  3. Obfuscated Code Contest
    By Stack Overflow in forum Contests Board
    Replies: 51
    Last Post: 01-21-2005, 03:17 PM
  4. Seems like correct code, but results are not right...
    By OmniMirror in forum C Programming
    Replies: 4
    Last Post: 02-13-2003, 12:33 PM
  5. bad code?
    By brad123 in forum C Programming
    Replies: 11
    Last Post: 05-02-2002, 03:38 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21