Use of std::string Aborts

Discussion in 'C++' started by Mike Copeland, Dec 16, 2011.

  1. 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;
    }
     
    Mike Copeland, Dec 16, 2011
    #1
    1. Advertising

  2. Mike Copeland

    Christopher Guest

    On Dec 16, 1:34 pm, (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...
     
    Christopher, Dec 16, 2011
    #2
    1. Advertising

  3. 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
     
    Alf P. Steinbach, Dec 16, 2011
    #3
  4. > 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, Dec 16, 2011
    #4
  5. In article <>,
    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<{{
     
    Mike Copeland, Dec 16, 2011
    #5
  6. In article <jcg7tv$25sa$>, d 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<}}
     
    Mike Copeland, Dec 16, 2011
    #6
  7. Mike Copeland <> wrote:
    > struct DEF_STRUCT
    > {
    > string eventName;
    > PHASEREC Phases[5];
    > } *Defs[9];


    Is there a reason that the objects *must* be allocated dynamically?
     
    Juha Nieminen, Dec 17, 2011
    #7
  8. Mike Copeland

    Jorgen Grahn Guest

    On Fri, 2011-12-16, Christopher wrote:
    > On Dec 16, 1:34 pm, (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 .
     
    Jorgen Grahn, Dec 18, 2011
    #8
  9. Mike Copeland

    red floyd Guest

    On 12/18/2011 9:08 AM, Jorgen Grahn wrote:
    > On Fri, 2011-12-16, Christopher wrote:
    >> On Dec 16, 1:34 pm, (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?
     
    red floyd, Dec 18, 2011
    #9
  10. > >> 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!
     
    Mike Copeland, Dec 19, 2011
    #10
  11. > > 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.
     
    Mike Copeland, Dec 19, 2011
    #11
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Replies:
    8
    Views:
    3,021
    Jhair Tocancipa Triana
    Sep 10, 2005
  2. Peter Jansson
    Replies:
    5
    Views:
    6,439
    Ivan Vecerina
    Mar 17, 2005
  3. Graham
    Replies:
    0
    Views:
    141
    Graham
    Aug 22, 2003
  4. Emil Horowitz

    Loop aborts on web server

    Emil Horowitz, May 13, 2009, in forum: Perl Misc
    Replies:
    10
    Views:
    243
    Xho Jingleheimerschmidt
    May 14, 2009
  5. Sri
    Replies:
    0
    Views:
    141
Loading...

Share This Page