Array of CString in VC++ 8?

[openmp2000]

Hello,

I need to migrate a project in VC6 to Visual Studio 2005 (VC8) and got a problem. Basically, I need an array of CString. The original code is:

CString* pLabel = (CString*) new BYTE[ nArraySize * sizeof(CString) ];
ConstructElements( pLabel, nArraySize );
// ...some string assignment of pLabel and other function calls

// the way the array is deleted is
delete[] (BYTE*)pLabel ;

Since ConstructElements() is not available in VC8, I have difficulty to find a way to replace it. Also, this array is used by many other functions or even other external projects so I can't change it to CStringArray or other forms.

Anyway, I try to modified the code in VC8 as:

CString* pLabel = (CString*) new BYTE[ nArraySize * sizeof(CString) ];
memset( (void*)pLabel, 0, nArraySize * sizeof(CString) );
for( int i = 0; i < nArraySize; i++, pTmp++ )
::new((void*)pTmp)CString; // call constructor

This change works OK in the projects. However, I got memory leak.

Can anybody help? Thank you very much.

[openmp2000]

Sorry, I missed a line in the new code, just before memset function call.

CString* pTmp = pLabel;

[Lindley]

Don't use memset! memset should not be used in conjunction with C++ objects unless you can be certain they're POD, which seems unlikely in the case of CString.....

Second, is there a reason why you're allocating BYTE arrays and then trying to construct the CStrings later? Why not just do

CString* pLabel = new CString[ nArraySize ];

....

delete[] pLabel;

?

Or, for that matter, why not make it even simpler on yourself and do

std::vector<CString> pLabel(nArraySize);

? (This can be used with functions taking CString*s pretty easily.)

[Skizmo]

CString* pLabel = (CString*) new BYTE[ nArraySize * sizeof(CString) ];

aaaaargh.... what is this ? You are scaring me...

1. never take the size of a class.
2. why are you creating a bytearray and then cast them to a string pointer ? These 2 have nothing to do with each other.

This is how you create a array of CString.

CString *pLabel = new CString[nArraySize];

memset( (void*)pLabel, 0, nArraySize * sizeof(CString) );

This destroys the class. You don't want to do that.

::new((void*)pTmp)CString; // call constructor

I don't even want to know how you got this idea...

[Lindley]

That's placement new.....it can work, but why go to all that trouble?

[Paul McKenzie]

CString* pLabel = (CString*) new BYTE[ nArraySize * sizeof(CString) ];
memset( (void*)pLabel, 0, nArraySize * sizeof(CString) );

You are programming in C++, so use C++, not 'C' or C-like concepts. You create arrays of objects by -- creating arrays of objects. What is all of this code you've written supposed to do? Are you purposefully trying to make your code convoluted so no one understands what you're doing?

What is wrong with just using CStringArray? Or just doing this?

#include <vector>
//...
typedef std::vector<CString> ArrayOfCString;

This change works OK in the projects. However, I got memory leak.

Then it isn't OK if it has memory leaks.

Your code defeats the entire purpose of CString, and that is to prevent you from having to allocate anything dynamically. You have containers that serve as dynamic arrays -- CStringArray being one, and std::vector<CString> being another. There is absolutely no need for overcomplicating something that is supposed to be simple.

Regards,

Paul McKenzie

[GCDEF]

Why not just use the CStringArray class?

And this

CString* pLabel = (CString*) new BYTE[ nArraySize * sizeof(CString) ];

makes no sense at all. A CString is not a BYTE. Why try to cast one to the other?

[Nikitozz]

The reason of the memory leak is simple. You call a constructor for your string

   ::new((void*)pTmp)CString;  // call constructor

,but you never call a destructor (which, by the way, frees all memory, allocated in the CString constructor).

The statement

delete[] (BYTE*)pLabel ;

won't call any destructor for your objects.

Any way I think it's wrong. I can't even imagine a situation, when this code is useful.

[VictorN]

Dear openmp2000!
I wonder, didn't you ever know CStringArray exists?

[Paul McKenzie]

Also, this array is used by many other functions or even other external projects so I can't change it to CStringArray or other forms.

CStringArray has been available since the beginning of MFC, so why didn't you use it to begin with? Did you not see in the MSDN documentation about CStringArray when you developed for VC 6?

Secondly, I have heard the excuse many times of "I can't change to using X because I have a lot of code". Personally, I have had to maintain applications that have hundreds of thousands of lines. Changing from one type to usage of another, and especially in the case such as you speak of, the change took less than a day.

All you have to do is just change the definition of one of your "main" variables from your home-made array to CStringArray. Then you recompile the source code and fix the compiler errors. Once the errors are fixed, the program shoud work as it always did after a round of tests. Sometimes, the change in source code is a simple search and replace, other times it is search and replace, with some changes in the source code to handle the new type (and the change is usually for the better -- the change usually shortens the code and makes it easier to maintain).

And lastly, even if you did want to make your own dynamic array of CString, it would look nothing like what you posted. Coding dynamic arrays, while error-prone, is still simple in concept, much more simple than your current code describes.

Regards,

Paul McKenzie

[Ajay Vijay]

CString* pLabel = (CString*) new BYTE[ nArraySize * sizeof(CString) ];

It looks like that you've assumption that CString is large enough to hold any number of characters in it. That means, you assume CString's size to be few KBs/MBs that wouldn't change by assigning different strings. Your assumed CString may look like:

class CString
{
char MyString[1000000]; // ANSI string
...
};

But, in reality, the implementation is something like:

class CString
{
char* pMyString; // Dynamic string
...
};

Which obviously doesn't state that CString can hold any size of string by itself. It would allocate some bytes, and 'pMyString' would point to it.

[openmp2000]

Thanks for all the comments. I will think about the options and test them in the new code.

I don't know the reason why it was in this way either. The code has been there for more than 10 years and the guy who wrote it left us already. But from the performance of our product in the last 10 years, this piece of code didn't give us problem. That is another reason I am reluctant to make big change on the original code.

Anyway, I will test the options. CStringArray may be my last option though. Thanks again.

[Lindley]

But from the performance of our product in the last 10 years, this piece of code didn't give us problem. That is another reason I am reluctant to make big change on the original code.

First: if you have a memory leak, then that's a problem.

However, it's true that when something is working it's reasonable to minimize the changes you make to it. Do you have a comprehensive test suite for this code? If so, run it ASAP after the change to make sure things are still good.

[Lindley]

I can't even imagine a situation, when this code is useful.

A typical implementation of std::vector will use placement new/placement delete internally, since there's no need to construct objects on the slots which are reserved but not in use.

However, it really should be kept "behind the scenes" as much as possible.

[Paul McKenzie]

I don't know the reason why it was in this way either. The code has been there for more than 10 years and the guy who wrote it left us already.

Anytime I see code like that, it gives a bad impression. The coder spent company time doing something that was completely unneccessary in terms of coding -- all they had to do is look at MSDN and use CStringArray -- problem solved. Or the company doesn't value its time? Your here now having to ask a question -- see how all of this wasted time adds up, all because for some reason, a refusal of using an MFC standard class wasn't done?

But from the performance of our product in the last 10 years, this piece of code didn't give us problem. That is another reason I am reluctant to make big change on the original code.

But you are introducing a function that does not exist in VC8, and the function has bugs. That is a big enough change right there.

The migration of VC6 to Visual Studio is the opportunity for you to slowly replace that code with CStringArray, one or two variables at a time. If the person(s) left already, then that is more of a reason to change to something standard (C++ or MFC standard). Or at least slowly transition to the CStringArray.

Regards,

Paul McKenzie