Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c++ > #12594 > unrolled thread

Use of std::string Aborts

Back to article view | Back to comp.lang.c++

Contents

  Use of std::string Aborts mrc2323@cox.net (Mike Copeland) - 2011-12-16 12:34 -0700
    Re: Use of std::string Aborts Leigh Johnston <leigh@i42.co.uk> - 2011-12-16 19:45 +0000
      Re: Use of std::string Aborts mrc2323@cox.net (Mike Copeland) - 2011-12-16 16:40 -0700
    Re: Use of std::string Aborts drew@furrfu.invalid (Drew Lawson) - 2011-12-16 19:58 +0000
      Re: Use of std::string Aborts mrc2323@cox.net (Mike Copeland) - 2011-12-16 16:42 -0700
    Re: Use of std::string Aborts Christopher <cpisz@austin.rr.com> - 2011-12-16 12:36 -0800
      Re: Use of std::string Aborts mrc2323@cox.net (Mike Copeland) - 2011-12-16 16:38 -0700
      Re: Use of std::string Aborts Jorgen Grahn <grahn+nntp@snipabacken.se> - 2011-12-18 17:08 +0000
        Re: Use of std::string Aborts red floyd <no.spam.here@its.invalid> - 2011-12-18 09:59 -0800
          Re: Use of std::string Aborts mrc2323@cox.net (Mike Copeland) - 2011-12-18 20:21 -0700
    Re: Use of std::string Aborts "Alf P. Steinbach" <alf.p.steinbach+usenet@gmail.com> - 2011-12-16 23:07 +0100
    Re: Use of std::string Aborts Juha Nieminen <nospam@thanks.invalid> - 2011-12-17 07:40 +0000
      Re: Use of std::string Aborts mrc2323@cox.net (Mike Copeland) - 2011-12-18 20:24 -0700

#12594 — Use of std::string Aborts

   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;
}

[toc] | [next] | [standalone]

#12595

On 16/12/2011 19:34, Mike Copeland wrote:
> #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]

/Leigh

[toc] | [prev] | [next] | [standalone]

#12601

In article <fqKdnWtwK7eZPXbTnZ2dnUVZ8nqdnZ2d@giganews.com>, 
leigh@i42.co.uk says...
> > #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<{{

[toc] | [prev] | [next] | [standalone]

#12596

In article <MPG.29554b332d4721d79896de@news.eternal-september.org>
	mrc2323@cox.net (Mike Copeland) writes:

>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;
>}


-- 
 Drew Lawson               So risk all or don't risk anything
			   You can lose all the same

[toc] | [prev] | [next] | [standalone]

#12602

In article <jcg7tv$25sa$1@raid.furrfu.com>, drew@furrfu.invalid says...
> >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<}}

[toc] | [prev] | [next] | [standalone]

#12597

On Dec 16, 1:34 pm, mrc2...@cox.net (Mike Copeland) wrote:
>     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...

[toc] | [prev] | [next] | [standalone]

#12600

> 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<}}

[toc] | [prev] | [next] | [standalone]

#12639

On Fri, 2011-12-16, Christopher wrote:
> On Dec 16, 1:34 pm, mrc2...@cox.net (Mike Copeland) wrote:
>>     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...

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

-- 
  // Jorgen Grahn <grahn@  Oo  o.   .     .
\X/     snipabacken.se>   O  o   .

[toc] | [prev] | [next] | [standalone]

#12641

On 12/18/2011 9:08 AM, Jorgen Grahn wrote:
> On Fri, 2011-12-16, Christopher wrote:
>> On Dec 16, 1:34 pm, mrc2...@cox.net (Mike Copeland) wrote:
>>>      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...
>
> 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?

[toc] | [prev] | [next] | [standalone]

#12647

> >> 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.)
> >
> 
> 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!

[toc] | [prev] | [next] | [standalone]

#12599

On 16.12.2011 20:34, Mike Copeland wrote:
>     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

[toc] | [prev] | [next] | [standalone]

#12611

Mike Copeland <mrc2323@cox.net> wrote:
> struct DEF_STRUCT
> {
>    string   eventName;
>    PHASEREC Phases[5];
> } *Defs[9];

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

[toc] | [prev] | [next] | [standalone]

#12648

> > 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.

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.c++

csiph-web