Allocating a string buffer

[Bob Altman]

Hi all,

I need to allocate a buffer large enough to store a copy of a null-terminated
string that is given to me as an argument. The code that I am using looks a lot
like old ugly C code. Is there a "better" C++ way to do this sort of thing (see
below)?

TIA - Bob

---

typedef struct ICInfo_tag {
char* Name;

// Constructor
ICInfo_tag(const char* name)
{
// Get a copy of name
size_t nameLen = strlen(name) + 1;
this->Name = static_cast<char*>(malloc(nameLen));
memcpy(this->Name, name, nameLen);
}
} ICInfo;

[Jeroen Mostert]

Bob Altman wrote:

I need to allocate a buffer large enough to store a copy of a null-terminated
string that is given to me as an argument. The code that I am using looks a lot
like old ugly C code. Is there a "better" C++ way to do this sort of thing (see
below)?

TIA - Bob

---

typedef struct ICInfo_tag {
char* Name;

// Constructor
ICInfo_tag(const char* name)
{
// Get a copy of name
size_t nameLen = strlen(name) + 1;
this->Name = static_cast<char*>(malloc(nameLen));
memcpy(this->Name, name, nameLen);
}
} ICInfo;

The canonical safer way would be to use std::string instead of a char* so
you don't have to worry about memory in the first place. However, I assume
"ICInfo" is a struct for a good reason. If it needs to be a Plain Old
Struct, you can't use std::string (or auto_ptr<>) and there's not much room
to improve on this. If it's a struct for no good reason, then by all means
make it a class instead.

You can consider making the constructor take a const std::string& instead of
a const char*. If the rest of your code consists of good C++ citizens who
all use strings too, this is more efficient than having to use strlen() all
the time.

Of course, the one improvement you can and should make is to use new[]
instead of malloc().

--
J.
http://symbolsprose.blogspot.com

[Bob Altman]

The canonical safer way would be to use std::string instead of a char* so you

don't have to worry about memory in the first place. However, I assume
"ICInfo" is a struct for a good reason.

ICInfo has a bunch of other members which I stripped out for this posting. I
simply never considered making it a class rather than a struct.

I allocate a bunch of ICInfo structures and store them in a map. Now that I
think about it, I could make ICInfo a class and the end result in my app would
be about the same.

If it needs to be a Plain Old Struct, you can't use std::string

Why not? Why can't I just say:

typedef struct ICInfo_tag {
// Use a string rather than a char*
std::string Name;
int SomeotherMember;

// Constructor
ICInfo_tag(const char* name)
{
// Get a copy of name
this->Name = name;
}
} ICInfo;

Of course, the one improvement you can and should make is to use new[] instead
of malloc().

So, instead of malloc, I would say

This->Name = new char[nameLen];

which has the advantage that it will throw an exception rather than making me
check to see if it succeeded (which my original code neglected to do)

[Jeroen Mostert]

Bob Altman wrote:

>The canonical safer way would be to use std::string instead of a char* so you
don't have to worry about memory in the first place. However, I assume
"ICInfo" is a struct for a good reason.

ICInfo has a bunch of other members which I stripped out for this posting. I
simply never considered making it a class rather than a struct.

I allocate a bunch of ICInfo structures and store them in a map. Now that I
think about it, I could make ICInfo a class and the end result in my app would
be about the same.

>If it needs to be a Plain Old Struct, you can't use std::string

Why not? Why can't I just say:

typedef struct ICInfo_tag {
// Use a string rather than a char*
std::string Name;

A Plain Old Struct (I should say Plain Old Data Structure, really) is
special in that it's "just like a C struct", i.e. no constructors, no
destructors, no class members. This is a fairly technical concept that's
sometimes necessary for interoperability (you can't hand over ICInfo structs
to C code if they contain a std::string). Not every struct is a PODS.

In your case, you don't need any of that. If you're not bound by such
restrictions, you should usually prefer std::string to char*, since it saves
you allocation headaches, and you should prefer "class" to "struct", since
the only difference in C++ is that the default access modifier for structs
is "public", whereas for classes it's "private". After adding the
appropriate modifier yourself there's no semantic difference, but it's
clearer for readers if you use "struct" only to emphasize that your class is
really not a class at all but a PODS.

int SomeotherMember;

// Constructor
ICInfo_tag(const char* name)
{
// Get a copy of name
this->Name = name;
}

Most people prefer this syntax:

ICInfo_tag(const char* name) : Name(name) { }

This is actually more efficient than your code because it directly
constructs Name instead of copying over a temporary. While that's usually a
micro-optimization that doesn't matter (though depending on the nature of
your objects, not always), C++ programmers obsess enough over
micro-optimization that most wouldn't be caught dead doing it the "slow"
way. :-)

>Of course, the one improvement you can and should make is to use new[] instead
of malloc().

So, instead of malloc, I would say

This->Name = new char[nameLen];

which has the advantage that it will throw an exception rather than making me
check to see if it succeeded (which my original code neglected to do)

There's more than that:

- You can mix malloc() and new[], but you have to make sure to call free()
or delete[] as appropriate (you can't mix up the pairings). Sticking with
new and new[] for everything is a lot less error-prone.

- new and new[] don't require casts.

- new and new[] can allocate objects. malloc() can only give you raw memory.

However, it's better still not to mess with explicitly allocated memory when
it's not necessary and use stack-allocated objects instead, relying on
destructors for cleanup. This is the well known RAII idiom (Resource
Acquisition Is Initialization, and conversely, destruction is resource
release). Plus, of course, std::string is a lot more convenient to work with
than char* if you have to do string manipulation.

--
J.
http://symbolsprose.blogspot.com

[Bob Altman]

you should prefer "class" to "struct", since the only difference in C++ is

that the default access modifier for structs is "public", whereas for classes
it's "private". After adding the appropriate modifier yourself there's no
semantic difference

Ok, lets assume I've changed ICInfo into a class, and I've replaced the "char*
name" with "string name". I define my map like this:

typedef map<int, ICInfoICInfoMap;

Then, when I add an item to the map like this

g_ICInfoMap[index] = ICInfo(name, 5)

I am actually constructing a new ICInfo on the stack and then invoking the
build-in copy constructor to copy all of its member variables to a new ICInfo
contained in the map. Is that (more or less) correct? I assume that this is
something equivalent to a shallow copy. Is there anything I need to deal with
if I have a class (such as string) as a member variable?

Now, I have the following code that gets data out of the map:

void SomeSub(ICInfoMap& icInfoMap) {
// Iterate through all of the entries in the ICInfoMap
for (ICInfoMap::iterator it = icInfoMap.Begin();
it != icInfoMap.End(); it++) {
// Get a reference to the ICInfo structure
ICInfo& info = it->second;

// Do something with the ICInfo reference
cout << info.SomethingElse << endl;
}

I assume that the whole exercise of incrementing the iterator and accessing the
object via "it->second" is about as efficient as possible. In particular, I'm
hoping that I'm not causing C++ to make any more copies of the class data behind
my back. Is that correct?

[Brian Muth]

If you are concerned about efficiency, consider using a vector<ICInforather than a map. You can still access elements of the
vector using an index, and the element reference is retrieved much faster.

By the way I also suggest a name other than ICInfo, such as CInfo. By convention ICInfo infers "interface belonging to the CInfo
object" which is misleading.

HTH

Brian