Use of std::string Aborts

[Mike Copeland]

In the program below I'm trying to assign initial values, but I get
runtime errors when I execute it. Specifically, the program aborts
wherever I assign a value to the std::string variables. I used to do
all this (without difficulty) with C-style strings, but when I changed
the code to use std:strings, the code gets runtime errors.
There must be some concept I don't understand here, and I'd
appreciate help in pointing out what I did wrong and how to correct it.
TIA

#include <string>
using namespace std;

struct PHASEREC
{
short phaseType;
short phaseNum;
string phaseDescr;
string phaseTitle;
};
struct DEF_STRUCT
{
string eventName;
PHASEREC Phases[5];
} *Defs[9];

int main()
{
int iii, jjj;
for(iii = 0; iii < 9; iii++)
{
if(Defs[iii] == NULL)
Defs[iii] = (DEF_STRUCT*)malloc (sizeof(DEF_STRUCT));
Defs[iii]->eventName = ""; // << abort here!!
} // for
for(jjj = 0; jjj < 5; jjj++)// initialize Phase info
{
Defs[jjj]->Phases[iii].phaseNum = 0;
Defs[jjj]->Phases[iii].phaseType = 0;
Defs[jjj]->Phases[iii].phaseDescr = ""; // << & here!!
Defs[jjj]->Phases[iii].phaseTitle = ""; // << & here!!
} // for
return true;
}

 

[Christopher]

    int iii, jjj;

Just a nit pick, but it would be good practice to use meaningful
names. When debugging other's code names like these are irritating.
Perhaps something like indexToDefinitions would be more appropriate.

Lots of people use i or j in a simple for loop, but I am not sure why
you would use iiii or jjjj here...

 

[Alf P. Steinbach]

In the program below I'm trying to assign initial values, but I get
runtime errors when I execute it. Specifically, the program aborts
wherever I assign a value to the std::string variables. I used to do
all this (without difficulty) with C-style strings, but when I changed
the code to use std:strings, the code gets runtime errors.
There must be some concept I don't understand here, and I'd
appreciate help in pointing out what I did wrong and how to correct it.

Drew Lawson has already answered the most basic point, "malloc()
allocates memory but does not initialize anything within it", but
without giving a fix.

Leigh Johnston gave a fix, "Try using new rather than malloc", but
without explaining why that would be good idea.

So, let's look at this thing.

#include<string>
using namespace std;

struct PHASEREC

Here you're into a bad habit. Since macro names do not respect scopes,
since they can easily cause parts of your source code to be /replaced/
with something else, it's a common convention to use ALL UPPERCASE NAMES
for macros. In order for this convention to work well at reducing the
number of name collisions, other names should better /not/ be ALL
UPPERCASE. Besides, using ALL UPPERCASE is like SHOUTING. It's
distracting VISUAL NOISE, which you want to avoid in your source code.

{
short phaseType;
short phaseNum;

Don't use optimizations like `short` instead of `int`, unless there is a
/very good reason/ for it. Like, some heavy reason. Otherwise, just do
`int`.

string phaseDescr;
string phaseTitle;
};
OK.


struct DEF_STRUCT
{
string eventName;
PHASEREC Phases[5];
} *Defs[9];

Uhm, globals are bad.

But since there's no specific usage I'm not sure what to recommend
instead here.

int main()
{
int iii, jjj;
for(iii = 0; iii< 9; iii++)
{
if(Defs[iii] == NULL)

You don't have to test that. Actually it's better to /not/ test that
it's NULL. That's because, while as a static variable the array is
guaranteed to be zero-initialized before anything else, if you were to
move into this function as a local, it would not be automatically
zero-initialized (it would then only be initialized if you specified the
initialization).

Defs[iii] = (DEF_STRUCT*)malloc (sizeof(DEF_STRUCT));

In C++ you should as a rule not use `malloc`. That's because as Drew
Lawson pointed out, it doesn't do initialization. The `calloc` function
does initialize to 0 (as I recall), but does not do C++ level
initialization.

A general fix is to instead use C++ `new`, like

Defs[iii] = new DEF_STRUCT();

The parenthesis at the end there says, "please value-initialize this
object", which for built-in types like `short` means to zero-initialize
(they get zeroed), and which for `string` means to default-initialize
(you get logically empty strings).

An alternative, if you absolutely want to keep on using `malloc`, is to
do the construction separately, after allocation:

#include <new>

//...

Defs[iii] = reinterpret_cast<DEF_STRUCT*>( malloc( sizeof
DEF_STRUCT ) );
::new( &Defs[iii]->eventName ) string();
for( int zzz = 0; zzz < 5; ++zzz )
{
::new( &Defs[iii]->Phases[zzz].phaseDescr ) string();
::new( &Defs[iii]->Phases[zzz].phaseTitle ) string();
}

Apart from its ugliness compared to the earlier simple `new` expression,
this approach means you're also responsible for explicitly destroying
those string instances (invoking their destructors), unless you want to
leak memory.

Defs[iii]->eventName = ""; //<< abort here!!

Yes, because as Drew Lawson noted, `eventName` has not been initialized
at all. It probably contains a pointer with indeterminate value, point
somewhere to the Andromeda galaxy. The assignment tries to use it: bang!

} // for
for(jjj = 0; jjj< 5; jjj++)// initialize Phase info
{
Defs[jjj]->Phases[iii].phaseNum = 0;
Defs[jjj]->Phases[iii].phaseType = 0;
Defs[jjj]->Phases[iii].phaseDescr = ""; //<< & here!!
Defs[jjj]->Phases[iii].phaseTitle = ""; //<< & here!!
} // for
return true;
}

By the way, as a further simplification and safety, instead of raw
arrays try using std::vector.

Cheers & hth.,

- Alf

 

[Mike Copeland]

Just a nit pick, but it would be good practice to use meaningful

names. When debugging other's code names like these are irritating.
Perhaps something like indexToDefinitions would be more appropriate.

Lots of people use i or j in a simple for loop, but I am not sure why
you would use iiii or jjjj here...

FWIW, this code was extracted from a much larger program, where I had
comments and (for me) meaningful names. In the interest of posting a
small amount of stuff, I squeezed out some things that I didn't want to
burden you all with by either having to explain or document.
In my code, though, I don't like to use single characters as indices
and such, because it's hard for me to trace such variables with editor
commands and visual inspection. Just personal style... 8<}}

 

[Mike Copeland]

#include<string>
using namespace std;

struct PHASEREC
{
short phaseType;
short phaseNum;
string phaseDescr;
string phaseTitle;
};
struct DEF_STRUCT
{
string eventName;
PHASEREC Phases[5];
} *Defs[9];

int main()
{
int iii, jjj;
for(iii = 0; iii< 9; iii++)
{
if(Defs[iii] == NULL)
Defs[iii] = (DEF_STRUCT*)malloc (sizeof(DEF_STRUCT));
Defs[iii]->eventName = ""; //<< abort here!!
} // for
for(jjj = 0; jjj< 5; jjj++)// initialize Phase info
{
Defs[jjj]->Phases[iii].phaseNum = 0;
Defs[jjj]->Phases[iii].phaseType = 0;
Defs[jjj]->Phases[iii].phaseDescr = ""; //<< & here!!
Defs[jjj]->Phases[iii].phaseTitle = ""; //<< & here!!
} // for
return true;
}

Try using new rather than malloc and replace Defs[jjj] with Defs[???]
and Phases[iii] with Phases[jjj]

Yes, thank you. Also, you honed in on a problem I created when I
extracted some pertinent code from my much larger program code so that I
could post enough to demonstrate the problem. What I had above was
logic that couldn't possibly work had it executed. 8<{{

 

[Mike Copeland]

struct PHASEREC
{
short phaseType;
short phaseNum;
string phaseDescr;
string phaseTitle;
};
struct DEF_STRUCT
{
string eventName;
PHASEREC Phases[5];
} *Defs[9];

int main()
{
int iii, jjj;
for(iii = 0; iii < 9; iii++)
{
if(Defs[iii] == NULL)
Defs[iii] = (DEF_STRUCT*)malloc (sizeof(DEF_STRUCT));

malloc() allocates memory but does not initialize anything within
it. So you have some memory for a std::string, but the content is
not meaningful.

If not changing anything else, you want:
Defs[iii] = new DEF_STRUCT;

That will result in the constructors executing.

Defs[iii]->eventName = ""; // << abort here!!
} // for
for(jjj = 0; jjj < 5; jjj++)// initialize Phase info
{
Defs[jjj]->Phases[iii].phaseNum = 0;
Defs[jjj]->Phases[iii].phaseType = 0;
Defs[jjj]->Phases[iii].phaseDescr = ""; // << & here!!
Defs[jjj]->Phases[iii].phaseTitle = ""; // << & here!!
} // for
return true;
}

Perfect! This was what I was seeking, and your explanation of my
error (the use of malloc) was very helpful. 8<}}

 

[Juha Nieminen]

struct DEF_STRUCT
{
string eventName;
PHASEREC Phases[5];
} *Defs[9];

Is there a reason that the objects *must* be allocated dynamically?

 

[Jorgen Grahn]

Just a nit pick, but it would be good practice to use meaningful
names. When debugging other's code names like these are irritating.
Perhaps something like indexToDefinitions would be more appropriate.

Lots of people use i or j in a simple for loop, but I am not sure why
you would use iiii or jjjj here...

But these /are/ simple 'for' loops, as far as I can tell.

(Except he uses the final 'iii' from the first loop in the second
loop, which seems strange.)

/Jorgen

 

[red floyd]

But these /are/ simple 'for' loops, as far as I can tell.

(Except he uses the final 'iii' from the first loop in the second
loop, which seems strange.)

I had noticed that. Isn't he indexing off the end of his array there?

 

[Mike Copeland]

Just a nit pick, but it would be good practice to use meaningful

I had noticed that. Isn't he indexing off the end of his array there?

Yes. This is because as I cut and pasted code from the (much larger)
actual program, I changed the loop variables to something that didn't
have to be explained in my post (simple names). In doing all that, I
failed to include the 2nd loop's code where it would more likely work:
in the first loop.
I would have found the error had I gotten past the first string
variable assignment - prior to the first loop - but the "malloc" error
prevented that code from being executed. <sigh...>
In any event, I got the needed information, and I fixed my program as
(really) needed. Thanks to all!

 

[Mike Copeland]

struct DEF_STRUCT

{
string eventName;
PHASEREC Phases[5];
} *Defs[9];

Is there a reason that the objects *must* be allocated dynamically?

Probably not. It's code conversion from an old Turbo Pascal system
from which my current code is derived. This system was only 16 bit, and
memory management was much more critical than my C/C++ applications.
I will look into this issue and see how much effort's involved in
fixing the objects. Thanks.