CArray debug exception

[TubularX]

I'm running VS2010, 64-bit Win7. I created a new MFC-project, clicked finish to accept the wizard's default settings and inserted this code into the mainframe constructor:

#include <vector>

CMainFrame::CMainFrame()
{	
	CArray< std::vector<int>, std::vector<int>> arr;
	
	std::vector<int> vec1;
	vec1.push_back(1);
	arr.Add( vec1 );
	
	std::vector<int> vec2;
	vec2.push_back(2);
	arr.Add( vec2 );

	std::vector<int> vec3;
	vec3.push_back(3);
	arr.SetAt( 0, vec3 );

	std::vector<int> vecFirst = arr.GetAt(0);
		
	CString output;	
	output.Format(_T("%d"), vecFirst.at(0));
	AfxMessageBox(output);
	
	// TODO: add member initialization code here
	theApp.m_nAppLook = theApp.GetInt(_T("ApplicationLook"), ID_VIEW_APPLOOK_VS_2008);
}

I expect to see a messagebox showing "3" when running the application. This works in release mode, but in debug mode throws "0xC0000005: Access violation".

Please, can someone explain why? Or is it just my computer?

[Philip Nicoletti]

I don't have access to VS2010 on this system, but the code looks OK.

1. What line causes the access violation ?

2. Although your declaration statement is valid, there is no reason
not to use:

CArray< std::vector<int>, const std::vector<int> & > arr;

3. Depending on what you are doing, you might also consider :

const std::vector<int> & vecFirst = arr.GetAt(0);

[Paul McKenzie]

I'm running VS2010, 64-bit Win7. I created a new MFC-project, clicked finish to accept the wizard's default settings and inserted this code into the mainframe constructor:

Well, you're not doing yourself a lot of favors by mixing up CArray and std::vector in the same application. What are you trying to accomplish by using both classes that are essentially the same thing?

Either use one or the other, not both. For example, instead of this:

	CArray< std::vector<int>, std::vector<int>> arr;

Use this:

std::vector<std::vector<int> > arr;

Second, when you debug your application, which line gives the exception?

Last, if you're doing these assignments:

std::vector<int> vecFirst = arr.GetAt(0);

then you should realize that CArray works differently than std::vector. Assigning a vector to a vector copies the entire vector from the right-hand side of the "=" to the left-hand side. The CArray::GetAt() does no such copying -- a reference is returned. This is why using CArray and std::vector in the same function, let alone the same application, will lead to mistakes being made, especially when it comes to assignment and copying.

Regards,

Paul McKenzie

[Philip Nicoletti]

Last, if you're doing these assignments:

std::vector<int> vecFirst = arr.GetAt(0);

then you should realize that CArray works differently than std::vector. Assigning a vector to a vector copies the entire vector from the right-hand side of the "=" to the left-hand side. The CArray::GetAt() does no such copying -- a reference is returned. This is why using CArray and std::vector in the same function, let alone the same application, will lead to mistakes being made, especially when it comes to assignment and copying.

I agree with not mixing CArray and std::vector, but I don't quite understand what you are
saying here. Even if "arr" was a std::vector, that line of code (with at() or operator[])
would end up doing a copy also. That is why I suggested a const reference in my
third point in my previous post.

[Paul McKenzie]

I agree with not mixing CArray and std::vector, but I don't quite understand what you are
saying here. Even if "arr" was a std::vector, that line of code (with at() or operator[])
would end up doing a copy also.

You're right, a copy is made.

However, it is the copy that is causing the problem. After debugging, the reason is that CArray only works properly for non-POD types. The reason is this:

   m_pData = (TYPE*) new BYTE[(size_t)nAllocSize * sizeof(TYPE)];
   memset((void*)m_pData, 0, (size_t)nAllocSize * sizeof(TYPE));

This is how CArray allocates its data for the type being stored. The m_pData is supposed to be the array of std::vector objects. Of course, this will not work for non-POD types, as non-POD types cannot be created and initialized this way.

The correct way is to do this:

m_pData = new TYPE[/*number of elements*/];

That's why operator =, where a CArray is on the right and a std::vector is on the left will not work. What winds up happening is that this line:

std::vector<int> vecFirst = arr.GetAt(0);

causes GetAt(0) to return a reference to an array of corrupted vector objects. Then vecFirst attempts to initialize itself with these bogus vectors, and we get the crash.

I don't know why Microsoft didn't fix their CArray implementation to just call operator new[] properly internally when creating objects. It wouldn't affect existing applications, as far as I know, and would have CArray work for non-POD types.

So maybe the OP shouldn't use CArray for storage of non-POD types and just use std::vector.

Regards,

Paul McKenzie

[Philip Nicoletti]

This is strange ... in the documentation, Microsoft says that the constructor
(and destructor) are called ...

http://support.microsoft.com/kb/318734

Under the "More Information" section.

[Philip Nicoletti]

Even stranger ...

http://msdn.microsoft.com/en-us/libr...v=VS.100).aspx

The note says:

Most methods that resize a CArray object or add elements to it use memcpy_s to move elements. This is a problem because memcpy_s is not compatible with any objects that require the constructor to be called. If the items in the CArray are not compatible with memcpy_s, you must create a new CArray of the appropriate size. You must then use CArray::Copy and CArray::SetAt to populate the new array because those methods use an assignment operator instead of memcpy_s.

The note is there for the 2008 and 2010 documentation, but not the 2005.


Also, in the original code, if the CArray is declared as I suggested in my first post,
there is no crash.

I admit it ... I'm confused.

[Paul McKenzie]

The note is there for the 2008 and 2010 documentation, but not the 2005.

I don't know why MS just couldn't get someone to enhance this to automatically work with non-POD types. It's all internal to CArray, and no existing app should or would have been affected by the change. It isn't even that difficult to implement.

Probably MS is not putting in any more time or effort into their MFC container classes...

Regards,

Paul McKenzie

[TubularX]

First, thanks to Philip for your reply.

1. The line "std::vector<int> vecFirst = arr.GetAt(0);" is causing the exception.

2. Agreed, sorry for not using const correctness in this example.

3. I already knew, I was just curious about the case I posted and looking for an explanation. It's dangerous since it's not detected during compile time, so quite easy to make such a mistake.

And thanks to Paul for pointing out the copy issue, but how/why does that differ in release mode where GetAt(0) appears to copy a valid vector?

So that means CArray is best avoided for non-POD types? I think many programmers are not aware of this.

As for the case with std::vector, like you said there is no reason to mix with CArray, this was just an example that puzzled me.

[Paul McKenzie]

And thanks to Paul for pointing out the copy issue, but how/why does that differ in release mode where GetAt(0) appears to copy a valid vector?

It's no different than if you declared an array witn n elements, and you write to element n+1. Does the program crash? It may, it may not.

When mistakes like this are made in a C++ program, there is no guarantee as to how a program is to behave. The program may seem to work, but is fatally flawed as shown, or it may work with one set of options and not work with another.

So that means CArray is best avoided for non-POD types?

Yes -- avoid CArray as a storage for non-POD types, unless you jump through all sorts of hoops and implement what is stated in the VS 2010 entry that Philip pointed out. The internal implementation of CArray when it comes to allocating the data proves that non-POD types cannot be used safely as the type to store in a CArray.

I think many programmers are not aware of this.

I wasn't aware until I saw the internals of the CArray class. Note that Philip mentioned that MSDN says nothing about this until VS 2008, meaning this issue has been flying under the radar until fairly recently (given that the MFC array classes have been around since the early 90's).

Regards,

Paul McKenzie

[TubularX]

Thanks again for your informative reply.

When rewriting to use std::vector instead, I miss the "SetAt" method from CArray. Is there any equivalent with bounds checking in STL to prevent writing to "element n+1" as you mentioned?

An assert will fail using the [] operator in debug mode but isn't safe in release. I would wish for an exception to be thrown in such cases, just like bounds checking works in the at()-method of std::vector.

[Philip Nicoletti]

Thanks again for your informative reply.

When rewriting to use std::vector instead, I miss the "SetAt" method from CArray. Is there any equivalent with bounds checking in STL to prevent writing to "element n+1" as you mentioned?

An assert will fail using the [] operator in debug mode but isn't safe in release. I would wish for an exception to be thrown in such cases, just like bounds checking works in the at()-method of std::vector.

You answered your own question ... use the at() member function.
It can be used on both the right and left side of the equal sign.