CODEWITHSUNDEEP
c++refactoring
rerun
rerun

Reputation: 25495

Looking for some refactoring advice

I have some code that I had to write to replace a function that was literally used thousands of times. The problem with the function was that return a pointer to a static allocated buffer and was ridiculously problematic. I was finally able to prove that intermittent high load errors were caused by the bad practice.

The function I was replacing has a signature of char * paddandtruncate(char *,int), char * paddandtruncate(float,int), or char * paddandtruncat(int,int). Each function returned a pointer to a static allocated buffer which was overwritten on subsequent calls.

I had three constants one the

  1. Code had to be replaceable with no impact on the callers.
  2. Very little time to fix the issue.
  3. Acceptable performance.

I wanted some opinion on the style and possible refactoring ideas.

The system is based upon fixed width fields padded with spaces, and has some architectural issues. These are not addressable since the size of the project is around 1,000,000 lines.

I was at first planning on allowing the data to be changed after creation, but thought that immutable objects offered a more secure solution.

    using namespace std;
class SYSTEM_DECLSPEC CoreString
{
private:
     friend ostream & operator<<(ostream &os,CoreString &cs);

     stringstream   m_SS          ;
     float          m_FltData     ;
     long           m_lngData     ;
     long           m_Width       ;
     string         m_strData     ;
     string                  m_FormatedData;
     bool           m_Formated    ;
     stringstream    SS            ;


public:

     CoreString(const string &InStr,long Width):
             m_Formated(false),
             m_Width(Width),
             m_strData(InStr)
             {
                     long OldFlags = SS.flags();
                     SS.fill(' ');
                     SS.width(Width);
                     SS.flags(ios::left);
                     SS<<InStr;
                     m_FormatedData = SS.str();
             }

     CoreString(long longData , long Width):
             m_Formated(false),
             m_Width(Width),
             m_lngData(longData)
             {
                     long OldFlags = SS.flags();
                     SS.fill('0');
                     SS.precision(0);
                     SS.width(Width);
                     SS.flags(ios::right);
                     SS<<longData;
                     m_FormatedData = SS.str();
             }

     CoreString(float FltData, long width,long lPerprecision):
             m_Formated(false),
             m_Width(width),
             m_FltData(FltData)
             {
                     long OldFlags = SS.flags();
                     SS.fill('0');
                     SS.precision(lPerprecision);
                     SS.width(width);
                     SS.flags(ios::right);
                     SS<<FltData;
                     m_FormatedData = SS.str();
             }

     CoreString(const string &InStr):
             m_Formated(false),
             m_strData(InStr)
             {
                     long OldFlags = SS.flags();
                     SS.fill(' ');
                     SS.width(32);
                     SS.flags(ios::left);
                     SS<<InStr;
                     m_FormatedData = SS.str();
             }
 public:
     operator const char   *() {return m_FormatedData.c_str();}
     operator const string& () const {return m_FormatedData;}
     const string& str() const ; 

};

const string& CoreString::str() const
{
     return m_FormatedData;
}

ostream & operator<<(ostream &os,CoreString &cs)
{
     os<< cs.m_Formated;
     return os;
 }

Upvotes: 0

Views: 204

Answers (5)

David Thornley
David Thornley

Reputation: 57036

I don't know how the callers are going to be using this, but allocating buffers using new into a auto_ptr<>s might work. It may satisfy criterion 1 (I can't tell without seeing the using code), and could be a pretty fast fix. The big issue is that it uses dynamic memory a lot, and that will slow things down. There's things you can do, using placement new and the like, but that may not be quick to code.

If you can't use dynamic storage, you're limited to non-dynamic storage, and there really isn't much you can do without using a rotating pool of buffers or thread-local buffers or something like that.

Upvotes: 1

Tim Sylvester
Tim Sylvester

Reputation: 23128

It sounds like the system is threaded, right? If it was simply a matter of it not being safe to call one of these functions again while you're still using the previous output, it should behave the same way every time.

Most compilers have a way to mark a variable as "thread-local data" so that it has a different address depending on which thread is accessing it. In gcc it's __thread, in VC++ it's __declspec(thread).

If you need to be able to call these functions multiple times from the same thread without overwriting the results, I don't see any complete solution but to force the caller to free the result. You could use a hybrid approach, where each thread has a fixed number of buffers, so that callers could make up to N calls without overwriting previous results, regardless of what other threads are doing.

Upvotes: 2

Steve Jessop
Steve Jessop

Reputation: 279225

The "intermittent high-load errors" are caused by race conditions where one thread tramples on the static buffer before another thread has finished using it, right?

So switch to using an output buffer per thread, using whatever thread-local storage mechanism your platform provides (Windows, I'm thinking).

There's no synchronisation contention, no interference between threads, and based on what you've said about the current implementation rotating buffers, almost certainly the calling code doesn't need to change at all. It can't be relying on the same buffer being used every time, if the current implementation uses multiple buffers.

I probably wouldn't design the API this way from scratch, but it implements your current API without changing it in a significant way, or affecting performance.

Upvotes: 0

Mark Ransom
Mark Ransom

Reputation: 308111

The code you've posted has a one huge problem - if a caller assigns the return value to a const char *, the compiler will make a silent conversion and destroy your temporary CoreString object. Now your pointer will be invalid.

Upvotes: 1

Mark Ransom
Mark Ransom

Reputation: 308111

If you really do mean "no impact on the callers", your choices are very limited. You can't return anything that needs to be freed by the caller.

At the risk of replacing one bad solution with another, the quickest and easiest solution might be this: instead of using a single static buffer, use a pool of them and rotate through them with each call of your function. Make sure the code that chooses a buffer is thread safe.

Upvotes: 2

Related Questions