0

Here's what I have:

void function() {
struct APROCS
{
DWORD PID;
TCHAR processname[256];
};  
static struct APROCS *aProcesses;
aProcesses = (APROCS *) calloc(8192, sizeof(APROCS));    

//do work...
[get PIDs and process names]
if ((aProcesses[i].PID > 4) && (_tcslen(aProcesses[i].processname) > 0
//do more work...    

free(aProcesses);aProcesses=NULL;
}

I'm looking to eliminate all bad code, and while this code works, it's probably not the correct way to do it. I basically setup an array of aProcesses[8192]. I want to use a class:

// CProcesses.h 
class CProcesses
{
public:
DWORD PID;
TCHAR processname[256];

CProcesses(void);
~CProcesses(void);
};

and

// CProcesses.cpp   
#include <Windows.h>
#include "CProcesses.h"
CProcesses::CProcesses(void)
{
}
CProcesses::~CProcesses(void)
{
}

Do I do this to allocate an array from the class?

void function() {
CProcesses *aProcesses[8192];

//do work...
[get PIDs and process names]
if ((aProcesses[i].PID > 4) && (_tcslen(aProcesses[i].processname) > 0
//do more work...  
}
Improve this question
4
  • You do know that in C++ the only difference between a struct and a class is the default visibility? For struct all fields are by default public while in a class they are private. That's it. Commented Mar 12, 2014 at 10:44
  • 1
    And don't allocate an array unless you know that you will use that amount of structures. Use std::vector instead. And instead of character arrays, use std::string. And a last hint, try to avoid pointers as much as you can. Commented Mar 12, 2014 at 10:46
  • How would I do the amount of stuctures using a vector? Commented Mar 12, 2014 at 12:09
  • That's the point, you don't. Just add a new structure when needed. Commented Mar 12, 2014 at 12:13

2 Answers 2

Reset to default
2

I'm looking to eliminate all bad code, and while this code works, it's probably not the correct way to do it.

So what's wrong with it? What needs to be fixed. It's not "bad" just because it uses the keyword struct instead of class. You can't fix broken code without knowing why it's broken.

A struct in C++ is simple a class where members are public by default, rather than private. So as is, your alternative implementation isn't any different.

You might regard the code as "bad" because it has a manual memory allocation with calloc that has to be explicitly freed. If an exception is thrown between your calls to calloc and free, you'll leak memory.

Since this is C++, why not keep the struct for the process (it's typical in C++ to use a struct for a pure lump of data with no methods) and use a class to manage the collection? You can either use a std::vector instead of a primitive array, or write your own class to manage the memory, allocating it in the constructor and freeing it in the destructor.

Using a vector is simplest and your less likely to make mistakes.

On the subject of vectors, it's safest to use STL containers as much as you can. e.g. use std::strings instead of char or TCHAR arrays/pointers for as long as possible, and only use the underlying char array when you need to pass it to the Win32 API.

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

Yes, bad because of the manual memory allocation. If I put "CProcesses aProcesses[8192];", without the '*', doesn't that create the class array on the stack? Because I have member 'processname' of a TCHAR of size 256, isn't 8192*256 bytes allocated on the stack? I do need to use TCHAR, btw. Thanks.
The TCHAR array is, but the APROCS array wasn't. I missed the CProcesses pointer array being stack allocated. But you only allocated pointers to CProcesses objects, you don't ever actually create those objects. CProcesses aProcesses[8192] will stack allocate the array correctly, but that is quite a large array to be allocated on the stack.
So if I put instead CProcesses *aProcesses[8192], that would allocate the array on the heap, and I'd have to free it later with delete[], yes? And how would I go about allocating the objects? I assume I need something after CProcesses *aProcesses[8192]?
No, that would create an array of pointers. To allocate the array on the heap you need to call new. CProcesses *aProcesses = new CProcesses[8192]; This is ill-advised though; if you must manually allocate it, it should be handled in its own class. Much safer to simply use a std::vector.
Is there any advantage to doing this versus my original code that just declared a struct and allocated the array with calloc? I mean, besides it's seems cleaner. And you mentioned using a vector instead of an array. Where would I put that in?
|
0

You should probably replace APROCS::processname with a std::string.

If your application is to be compiled only with TCHAR=char, you can use std::string. If it is to be compiled with TCHAR=wchar_t, consider using std::wstring.

You should also replace aProcesses with a std::vector.

This is not mandatory, but probably should make your data private in CProcesses and use accessors for it (or leave it all as a struct with public data.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.