Prev: Function vs Method
Next: C-type casting
From: Jochen Kalmbach [MVP] on 16 Dec 2009 09:38
Hi Stephen!

> Oh yes it does. POD'ness does not come into it. Yes I know there is no destructors
>
> delete [] retrieves the number of items
> delete does not.

In the current CRT/MS implementation it makes no difference.
But you are right: it is bad code and might break in the future...

--
Greetings
Jochen

My blog about Win32 and .NET
http://blog.kalmbachnet.de/
From: Stephen Howe on 16 Dec 2009 09:42
On Wed, 16 Dec 2009 20:12:13 +0800, "Jack" <jl(a)knight.com> wrote:

>Hi Stephen,
>Actually, I have tried both delete and delete[], but doesn't work either.
>Thanks for your help!!
>Jack

Your looking in the wrong place.
If delete or delete [] does not "work", the heap may have been corrupted some time ago.
So it is the previous code that uses the heap that needs looking at.

A program is like a wild west gunfighter
A program may "die" a long way from where it is "shot"
Debugging is observing the "death" ofa program and tracing backwards to where the first program error occured.
Programmers should remember that an error may cause subsequent errors and you may be looking at a subsequent error, not the
original causing error. So your delete may not be the original problem. You need to trace backwards, find the first place that
the heap is corrupted, fix that.

Sprinkle your debug code with _heapchk()
It should return _HEAPOK or _HEAPEMPTY

If it returns any other value, you have corrupted the heap between this call and the last good call to _heapchk()

Note, it is an error to

1. Call free(), delete or delete [] on memory more than once on the same pointer except if NULL
2. Call free() on memory not obtained by malloc(), calloc() or realloc()
3. Call delete on memory not obtained by new
4. Call delete [] on memory not obtained by new []
5. Read or writing to memory after free(), delete or delete [] has been called.
6. Writing after the end of block obtained by new, new[], malloc(), calloc() or realloc()
7. Writing before the beginning of a block obtained by new, new[], malloc(), calloc() or realloc()
8. Supplying realloc() a pointer that is not NULL and not obtained by realloc(), calloc() or malloc()

Stephen Howe
From: Stephen Howe on 16 Dec 2009 11:37
>In the current CRT/MS implementation it makes no difference.

That is probably an optimisation.
The CRT/MS arranges that operator delete is called for array PODs, knowing that it makes no difference and it is a tad faster.
But the programmer should not assume that.

Stephen Howe
From: Giovanni Dicanio on 16 Dec 2009 12:27
"Jack" <jl(a)knight.com> ha scritto nel messaggio
news:OcE9ivkfKHA.5784(a)TK2MSFTNGP05.phx.gbl...

> HRESULT CMesh::SetMeshData(char *szfilename)

You should respect const-correctness, and use 'const char *' as input
parameter.
Better, move to Unicode (or at least use TCHAR, which works in both
ANSI/MBCS and Unicode builds).
Moreover, usual Hungarian Notation for pointers to "raw C" NUL-terminated
strings is *psz*, not 'sz'; 'sz' is usually for static buffers like:

char szBuf[ 100 ];

(in fact, sizeof(pszSomething) is always 4 on 32-bit systems, i.e. the size
of a pointer; instead sizeof(szSomething) is the size in bytes of given
buffer; so it can be good to differentiate between these two cases using
different prefixes).
So, I would write:

HRESULT CMesh::SetMeshData( LPCTSTR pszFilename )


> {
> LARGE_INTEGER size;
> HANDLE hFileMapping;
> int cchFileName;
> char szPath[256];
> char *szTemp;
> DWORD cchPath;

cchFileName, szTemp and cchPath seem not used in your code: I would just
remove their definitions from method body.
Moreover, use WCHAR or TCHAR, instead of obsolete char, e.g.:

TCHAR szPath[ ... ];

> CAllocateHierarchy Alloc;
>
> // unload these functions to a dll
> ::GetModuleFileNameA(NULL, szPath, sizeof(szPath));

Just use the Unicode or TCHAR versions of Win32 APIs (and use _countof or
ARRAYSIZE instead of sizeof, to get proper size in *elements count*, not in
bytes):

GetModuleFileName( NULL, szPath, _countof( szPath ) );

Moreover, I would check the return value of Win32 APIs like the above one.

>
> strcat (szPath, szfilename);

Use CString class to concatenate strings, or use safe string functions like
StringCchCat, but you should avoid use of old strcat & Co.

>
>
> //// Open .dat mesh
> HANDLE h = CreateFileA(szPath, GENERIC_READ, NULL, NULL, OPEN_EXISTING,

CreateFile is just fine (get rid of the CreateFileA).

> FILE_ATTRIBUTE_NORMAL, NULL);
> if (h == INVALID_HANDLE_VALUE)
> {
> MessageBoxA(NULL, "Couldn't open file with CreateFile()", "Error",
> MB_OK);
> return E_FAIL;

I think that it is better to return an HRESULT based on GetLastError() than
a generic E_FAIL.
e.g.

DWORD errorCode = GetLastError();
return HRESULT_FROM_WIN32( errorCode );

Moreover, I would prefer tracing error messages like above one using
ATLTRACE instead of MessageBox.
All that applies also to other Win32 calls you had (like CreateFileMapping
and MapViewOfFile).

> }
>
> GetFileSizeEx(h, &size);

I would check the return value of GetFileSizeEx, too.

>
> std::vector< BYTE > v( size.QuadPart+0x1e0-4 );
>

Note that there is a subtle thing here.
In case of allocation failure, std::vector throws a C++ exception (I think
std::bad_alloc).
But you are returning HRESULT's.
So, I would safely guard the vector construction using a try/catch, and in
case of std::bad_alloc exception return a proper HRESULT code like
E_OUTOFMEMORY.

> BYTE * binfile = &v[0];
>
>
> //PBYTE binfile = new BYTE[size.QuadPart+0x1e0-4]; // maybe get the file
> size 1st
> memset (binfile, 0, size.QuadPart+0x1e4-4);

I don't like these "magic" numbers like 0x1E4 - 4.
I would define a named constant for that:

static const size_t something = 0x1E0;

Moreover, you passed 'size.QuadPart+0x1e0-4' to the constructor, but you are
memset'ting a different (bigger size).
I would just use v.size() method to get size of data in bytes (considering
that your vector instance is a vector of BYTE's).

memset( &v[0], 0, v.size() );



> //binfile.clear();
> memcpy (binfile, template_bin, 0x1e0);
> //binfile.push_back(template_bin);//, 0x1e0);
> binfile += 0x1e0;
>
> hFileMapping = CreateFileMapping (h, NULL, PAGE_READONLY, 0, 0, NULL);
> if (hFileMapping == 0)
> {
> CloseHandle(h);
> MessageBoxA(NULL, "Couldn't open file mapping", "Error", MB_OK);
>
> return E_FAIL;
> }
>
> //// Get a whole file into memory
> PBYTE g_pMappedFileBase = (PBYTE) MapViewOfFile (hFileMapping,

Usually the g_ prefix is for *g*lobal variables, not for local ones.
Using g_ prefix for a local variable seems to me confusing.

> FILE_MAP_READ, 0, 0, 0);
> if (g_pMappedFileBase == 0)
> {
> CloseHandle (hFileMapping);
> CloseHandle (h);
> return E_FAIL;
> }
>
> if (memcmp(g_pMappedFileBase, "hdr1", 4) == 1)
> {
> CloseHandle(hFileMapping);
> CloseHandle(h);
> MessageBoxA(NULL, "Error Loading Header", "Error", MB_OK);
> return E_FAIL;
> }
>
> memcpy (binfile, g_pMappedFileBase+4, size.QuadPart-4);
> binfile -= 0x1e0; // back to origin
>
> HRESULT hr = D3DXLoadMeshHierarchyFromXInMemory((LPCVOID) binfile,
> size.QuadPart + 0x1e0-4, D3DXMESH_MANAGED, m_pDevice, &Alloc,
> NULL, (LPD3DXFRAME*)&(m_pFrameRoot), &m_pAnimController);
>
>
>
> if (FAILED(hr))
> {
> MessageBoxA(NULL, "Can't load mesh", "Error", MB_OK);
> }

You should return 'hr' here, not continuing execution to 'return S_OK;' that
follows.

>
>
> if (binfile) {
>
> delete binfile;
> binfile = NULL;

std::vector deletes itself (RAII): the above delete is wrong.
Note that you may want to use RAII paradigma also for file handle and memory
mapping handle, i.e. you could write a simple class to wrap these handles
and the destructor calls CloseHandle for you.
In this way, you don't need to repeat CloseHandle calls in every failure
exit points in your function: destructors will automatically be called for
you, and your code is simplified.

>
> }
>
>
> return S_OK;
>
>
> }


Giovanni


From: Bo Persson on 16 Dec 2009 12:45
Jochen Kalmbach [MVP] wrote:
> Hi Jack!
>
>> Actually, I have tried both delete and delete[], but doesn't work
>> either.
>
> In this special case (POD) it makes no difference...
>

This is also specific for the MS compilers, and not a general rule.


Bo Persson


First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5 6
Prev: Function vs Method
Next: C-type casting