Null Argument shouldnt be?

Discussion in 'C++' started by A. Anderson, Jun 28, 2007.

  1. A. Anderson

    A. Anderson Guest

    Howdy everyone,

    I'm experiencing a problem with a program that I'm developing. Take a
    look at this stack report from GDB -

    #0 0xb7d782a3 in strlen () from /lib/tls/i686/cmov/libc.so.6
    #1 0xb7d4c2f7 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
    #2 0xb7d6441b in vsprintf () from /lib/tls/i686/cmov/libc.so.6
    #3 0x08049ba0 in character_data::printf (this=0x800, argument=0x0)
    at character.c:198
    #4 0x0804a956 in do_look (ch=0x800, argument=0x8659164 "") at
    command.c:1062
    #5 0x0804ce7d in descriptor_data::interpret (this=0x8656e30,
    argument=0x805bde0 "look") at descriptor.c:278
    #6 0x0804d945 in descriptor_data::process_input (this=0x8656e30)
    at descriptor.c:207
    #7 0x0804bc08 in main_loop () at core.c:132
    #8 0x0804c080 in main (argc=2, argv=0xbf8cd4c4) at core.c:34

    And here are the relevant functions:

    void do_look( CHARACTER_DATA *ch, char *argument )
    {
    ROOM_DATA *room;
    CHARACTER_DATA *ich;

    if( !argument || argument == NULL || argument[0] == '\0' )
    {
    if(( room = ch->in_room ) == NULL )
    {
    ch->printf( "It doesnt seem that you are in a room. Please contact
    an administrator.\n\r" );
    return;
    }
    else
    {
    ch->printf( "%s\n\r", room->name );
    ch->printf( "-------------------------\n\r" );
    if( !room->description || room->description[0] == '\0' )
    {
    }
    else
    {
    ch->printf( "%s\n\r", room->description );
    ch->printf( "-------------------------\n\r" );
    }
    for( ich = room->first_character; ich; ich = ich->next )
    {
    if( ich != ch && ich->descriptor && ich->descriptor != NULL && ich-
    >descriptor->connected == CON_PLAYING )

    {
    ch->printf( "%s\n\r", ich->long_desc( ));
    }
    else
    {
    continue;
    }
    continue;
    }
    return;
    }
    return;
    }
    else
    {
    do_look( ch, "" );
    return;
    }
    return;
    }

    void character_data::printf( char *argument, ... )
    {
    DESCRIPTOR_DATA *descriptor;
    char buf[MAX_STRING_LENGTH];
    va_list arguments;

    if( !argument || argument == NULL || argument[0] == '\0' )
    {
    return;
    }
    else
    {
    va_start( arguments, argument );
    vsprintf( buf, argument, arguments );
    va_end( arguments );

    if(( descriptor = this->descriptor ) == NULL )
    {
    return;
    }
    else
    {
    descriptor->printf( buf );
    return;
    }
    return;
    }
    return;
    }

    I've reviewed this a couple dozen times and don't see a problem.
    Granted I lack a bit of experience but at about 35000 lines of code
    under my belt, and as many warnings or errors as you can come up with
    everywhere in between, I would typically see the problem here. If
    anyone has any suggestions as to why I'de be experiencing this sort of
    a problem I'de appreciate any suggestions. Perhaps a bad
    initialization? Perhaps something else. This is why I'm here. And if
    anyone has any constructive criticism for me as far as my code is
    related I would appreciate it. Thanks.
     
    A. Anderson, Jun 28, 2007
    #1
    1. Advertising

  2. A. Anderson

    A. Anderson Guest

    On Jun 28, 12:14 am, "A. Anderson" <>
    wrote:
    > Howdy everyone,
    >
    > I'm experiencing a problem with a program that I'm developing. Take a
    > look at this stack report from GDB -
    >
    > #0 0xb7d782a3 in strlen () from /lib/tls/i686/cmov/libc.so.6
    > #1 0xb7d4c2f7 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
    > #2 0xb7d6441b in vsprintf () from /lib/tls/i686/cmov/libc.so.6
    > #3 0x08049ba0 in character_data::printf (this=0x800, argument=0x0)
    > at character.c:198
    > #4 0x0804a956 in do_look (ch=0x800, argument=0x8659164 "") at
    > command.c:1062
    > #5 0x0804ce7d in descriptor_data::interpret (this=0x8656e30,
    > argument=0x805bde0 "look") at descriptor.c:278
    > #6 0x0804d945 in descriptor_data::process_input (this=0x8656e30)
    > at descriptor.c:207
    > #7 0x0804bc08 in main_loop () at core.c:132
    > #8 0x0804c080 in main (argc=2, argv=0xbf8cd4c4) at core.c:34
    >
    > And here are the relevant functions:
    >
    > void do_look( CHARACTER_DATA *ch, char *argument )
    > {
    > ROOM_DATA *room;
    > CHARACTER_DATA *ich;
    >
    > if( !argument || argument == NULL || argument[0] == '\0' )
    > {
    > if(( room = ch->in_room ) == NULL )
    > {
    > ch->printf( "It doesnt seem that you are in a room. Please contact
    > an administrator.\n\r" );
    > return;
    > }
    > else
    > {
    > ch->printf( "%s\n\r", room->name );
    > ch->printf( "-------------------------\n\r" );
    > if( !room->description || room->description[0] == '\0' )
    > {
    > }
    > else
    > {
    > ch->printf( "%s\n\r", room->description );
    > ch->printf( "-------------------------\n\r" );
    > }
    > for( ich = room->first_character; ich; ich = ich->next )
    > {
    > if( ich != ch && ich->descriptor && ich->descriptor != NULL && ich->descriptor->connected == CON_PLAYING )
    >
    > {
    > ch->printf( "%s\n\r", ich->long_desc( ));
    > }
    > else
    > {
    > continue;
    > }
    > continue;
    > }
    > return;
    > }
    > return;
    > }
    > else
    > {
    > do_look( ch, "" );
    > return;
    > }
    > return;
    >
    > }
    >
    > void character_data::printf( char *argument, ... )
    > {
    > DESCRIPTOR_DATA *descriptor;
    > char buf[MAX_STRING_LENGTH];
    > va_list arguments;
    >
    > if( !argument || argument == NULL || argument[0] == '\0' )
    > {
    > return;
    > }
    > else
    > {
    > va_start( arguments, argument );
    > vsprintf( buf, argument, arguments );
    > va_end( arguments );
    >
    > if(( descriptor = this->descriptor ) == NULL )
    > {
    > return;
    > }
    > else
    > {
    > descriptor->printf( buf );
    > return;
    > }
    > return;
    > }
    > return;
    >
    > }
    >
    > I've reviewed this a couple dozen times and don't see a problem.
    > Granted I lack a bit of experience but at about 35000 lines of code
    > under my belt, and as many warnings or errors as you can come up with
    > everywhere in between, I would typically see the problem here. If
    > anyone has any suggestions as to why I'de be experiencing this sort of
    > a problem I'de appreciate any suggestions. Perhaps a bad
    > initialization? Perhaps something else. This is why I'm here. And if
    > anyone has any constructive criticism for me as far as my code is
    > related I would appreciate it. Thanks.


    Just FYI, anyone that responds - line 1062 of command.c is 'ch-
    >printf( "%s\n\r", room->description );', in do_look.

    Character.c on 198 is the vsprintf in character_data::printf. Thanks
    again.
     
    A. Anderson, Jun 28, 2007
    #2
    1. Advertising

  3. A. Anderson

    Andre Kostur Guest

    Why is this code so C-like? Other than being in classes, it's all very
    C-like. There's no use of anything from C++ (other than being class
    member functions).

    Inline:


    "A. Anderson" <> wrote in
    news::

    > Howdy everyone,
    >
    > I'm experiencing a problem with a program that I'm developing. Take a
    > look at this stack report from GDB -
    >
    > #0 0xb7d782a3 in strlen () from /lib/tls/i686/cmov/libc.so.6
    > #1 0xb7d4c2f7 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
    > #2 0xb7d6441b in vsprintf () from /lib/tls/i686/cmov/libc.so.6
    > #3 0x08049ba0 in character_data::printf (this=0x800, argument=0x0)
    > at character.c:198
    > #4 0x0804a956 in do_look (ch=0x800, argument=0x8659164 "") at
    > command.c:1062
    > #5 0x0804ce7d in descriptor_data::interpret (this=0x8656e30,
    > argument=0x805bde0 "look") at descriptor.c:278
    > #6 0x0804d945 in descriptor_data::process_input (this=0x8656e30)
    > at descriptor.c:207
    > #7 0x0804bc08 in main_loop () at core.c:132
    > #8 0x0804c080 in main (argc=2, argv=0xbf8cd4c4) at core.c:34


    I would suspect your problem is way back at descriptor_data::interpret.
    The next call (do_look) claims that it has been passed a ch pointer to
    memory address 0x800. This is suspicious since it is nowhere near any of
    the other object addresses (such as 0x8656e30 for the descriptor_data. I
    am assuming that both objects are dynamically allocated). Check in
    interpret to see where you're getting that ch pointer from. It's
    probably messed up.

    >
    > And here are the relevant functions:
    >
    > void do_look( CHARACTER_DATA *ch, char *argument )
    > {
    > ROOM_DATA *room;
    > CHARACTER_DATA *ich;
    >
    > if( !argument || argument == NULL || argument[0] == '\0' )


    Why are you testing for both "!argument" and "argument == NULL"? They
    both do the same thing. (If argument == NULL, then !argument will
    evaluate to true as well...). And, argument doesn't appear to be used
    anywhere else in the function, and should probably be a "const
    std::string &" anyway.

    > {


    The room variable should be moved to here. It's not used outside of this
    if clause. Keep variables to the minimum scope.

    > if(( room = ch->in_room ) == NULL )


    You haven't checked to see if ch is a NULL pointer or not. This isn't
    safe. If your assumption is that ch will never be NULL, then make ch a
    CHARACTER_DATA& instead of CHARACTER_DATA*.

    > {
    > ch->printf( "It doesnt seem that you are in a room.
    > Please contact
    > an administrator.\n\r" );
    > return;
    > }
    > else
    > {
    > ch->printf( "%s\n\r", room->name );
    > ch->printf( "-------------------------\n\r" );
    > if( !room->description || room->description[0] == '\0'
    > ) {


    root->description should probably be a std::string too.

    > }
    > else
    > {
    > ch->printf( "%s\n\r", room->description );
    > ch->printf( "-------------------------\n\r" );
    > }
    > for( ich = room->first_character; ich; ich = ich->next
    > ) {


    ich should be declared inside the above for statement. It also is not
    used anywhere else. Keep variables to the minimum scope. And why bother
    implementing your own list when you can simply use std::list<> (or
    perhaps one of the other containers)?

    > if( ich != ch && ich->descriptor &&
    > ich->descriptor != NULL && ich-
    >>descriptor->connected == CON_PLAYING )


    Same as above, why test for both "ich->descriptor" and "ich->descriptor !
    = NULL" when they're saying the same thing?

    > {
    > ch->printf( "%s\n\r", ich->long_desc( ));
    > }
    > else
    > {
    > continue;


    continue what? The for loop that has no code after this anyway? Drop
    this else clause altogether. It's just extra code that you don't need to
    read.

    > }
    > continue;


    Same here. You're at the end of the for loop anyway. Drop the continue.
    It's just extra code that you don't need to read.

    > }
    > return;


    Why return here? There's nothing else that happens after this. Drop the
    return. It's just extra code that you don't need to read.

    > }
    > return;


    Why return here? There's nothing else that happens after this. Drop the
    return. It's just extra code that you don't need to read.

    > }
    > else
    > {
    > do_look( ch, "" );
    > return;
    > }
    > return;
    > }
    >
    > void character_data::printf( char *argument, ... )
    > {
    > DESCRIPTOR_DATA *descriptor;
    > char buf[MAX_STRING_LENGTH];
    > va_list arguments;
    >
    > if( !argument || argument == NULL || argument[0] == '\0' )
    > {


    Same as above. Why specify both tests when they're testing the same
    thing? And why not flip the boolean test around, then you don't need an
    else clause.

    > return;
    > }
    > else
    > {
    > va_start( arguments, argument );
    > vsprintf( buf, argument, arguments );
    > va_end( arguments );
    >
    > if(( descriptor = this->descriptor ) == NULL )
    > {
    > return;
    > }
    > else
    > {
    > descriptor->printf( buf );
    > return;
    > }


    Why not flip the test around, and why bother assigning it to a variable?
    And why did you choose to use the same name as what appears to be a
    member variable? Why not:

    if (descriptor != NULL)
    {
    descriptor->printf(buf);
    }


    > return;
    > }
    > return;


    You don't need any of these return statements. They're just extra code
    that you don't need to read.

    > }
    >
    > I've reviewed this a couple dozen times and don't see a problem.
    > Granted I lack a bit of experience but at about 35000 lines of code
    > under my belt, and as many warnings or errors as you can come up with
    > everywhere in between, I would typically see the problem here. If
    > anyone has any suggestions as to why I'de be experiencing this sort of
    > a problem I'de appreciate any suggestions. Perhaps a bad
    > initialization? Perhaps something else. This is why I'm here. And if
    > anyone has any constructive criticism for me as far as my code is
    > related I would appreciate it. Thanks.


    I'd recommend learning more C++. Right now it appears that you're using
    C++ as "C with funky structs that have functions attached to them".
    You're not using some very useful things, like references, std::string,
    and std::list.

    And all of your source filenames end in ".c". Tends to indicate that the
    files contain C code. But since it appears to be compiling, you probably
    are invoking the correct compiler (g++ vs. gcc). Popular convention
    would have the source files named ".cpp'.
     
    Andre Kostur, Jun 28, 2007
    #3
  4. A. Anderson

    A. Anderson Guest

    On Jun 28, 7:20 am, Andre Kostur <> wrote:
    > Why is this code so C-like? Other than being in classes, it's all very
    > C-like. There's no use of anything from C++ (other than being class
    > member functions).
    >
    > Inline:
    >
    > "A. Anderson" <> wrote innews::
    >
    >
    >
    >
    >
    > > Howdy everyone,

    >
    > > I'm experiencing a problem with a program that I'm developing. Take a
    > > look at this stack report from GDB -

    >
    > > #0 0xb7d782a3 in strlen () from /lib/tls/i686/cmov/libc.so.6
    > > #1 0xb7d4c2f7 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
    > > #2 0xb7d6441b in vsprintf () from /lib/tls/i686/cmov/libc.so.6
    > > #3 0x08049ba0 in character_data::printf (this=0x800, argument=0x0)
    > > at character.c:198
    > > #4 0x0804a956 in do_look (ch=0x800, argument=0x8659164 "") at
    > > command.c:1062
    > > #5 0x0804ce7d in descriptor_data::interpret (this=0x8656e30,
    > > argument=0x805bde0 "look") at descriptor.c:278
    > > #6 0x0804d945 in descriptor_data::process_input (this=0x8656e30)
    > > at descriptor.c:207
    > > #7 0x0804bc08 in main_loop () at core.c:132
    > > #8 0x0804c080 in main (argc=2, argv=0xbf8cd4c4) at core.c:34

    >
    > I would suspect your problem is way back at descriptor_data::interpret.
    > The next call (do_look) claims that it has been passed a ch pointer to
    > memory address 0x800. This is suspicious since it is nowhere near any of
    > the other object addresses (such as 0x8656e30 for the descriptor_data. I
    > am assuming that both objects are dynamically allocated). Check in
    > interpret to see where you're getting that ch pointer from. It's
    > probably messed up.
    >
    >
    >
    > > And here are the relevant functions:

    >
    > > void do_look( CHARACTER_DATA *ch, char *argument )
    > > {
    > > ROOM_DATA *room;
    > > CHARACTER_DATA *ich;

    >
    > > if( !argument || argument == NULL || argument[0] == '\0' )

    >
    > Why are you testing for both "!argument" and "argument == NULL"? They
    > both do the same thing. (If argument == NULL, then !argument will
    > evaluate to true as well...). And, argument doesn't appear to be used
    > anywhere else in the function, and should probably be a "const
    > std::string &" anyway.
    >
    > > {

    >
    > The room variable should be moved to here. It's not used outside of this
    > if clause. Keep variables to the minimum scope.
    >
    > > if(( room = ch->in_room ) == NULL )

    >
    > You haven't checked to see if ch is a NULL pointer or not. This isn't
    > safe. If your assumption is that ch will never be NULL, then make ch a
    > CHARACTER_DATA& instead of CHARACTER_DATA*.
    >
    > > {
    > > ch->printf( "It doesnt seem that you are in a room.
    > > Please contact
    > > an administrator.\n\r" );
    > > return;
    > > }
    > > else
    > > {
    > > ch->printf( "%s\n\r", room->name );
    > > ch->printf( "-------------------------\n\r" );
    > > if( !room->description || room->description[0] == '\0'
    > > ) {

    >
    > root->description should probably be a std::string too.
    >
    > > }
    > > else
    > > {
    > > ch->printf( "%s\n\r", room->description );
    > > ch->printf( "-------------------------\n\r" );
    > > }
    > > for( ich = room->first_character; ich; ich = ich->next
    > > ) {

    >
    > ich should be declared inside the above for statement. It also is not
    > used anywhere else. Keep variables to the minimum scope. And why bother
    > implementing your own list when you can simply use std::list<> (or
    > perhaps one of the other containers)?
    >
    > > if( ich != ch && ich->descriptor &&
    > > ich->descriptor != NULL && ich-
    > >>descriptor->connected == CON_PLAYING )

    >
    > Same as above, why test for both "ich->descriptor" and "ich->descriptor !
    > = NULL" when they're saying the same thing?
    >
    > > {
    > > ch->printf( "%s\n\r", ich->long_desc( ));
    > > }
    > > else
    > > {
    > > continue;

    >
    > continue what? The for loop that has no code after this anyway? Drop
    > this else clause altogether. It's just extra code that you don't need to
    > read.
    >
    > > }
    > > continue;

    >
    > Same here. You're at the end of the for loop anyway. Drop the continue.
    > It's just extra code that you don't need to read.
    >
    > > }
    > > return;

    >
    > Why return here? There's nothing else that happens after this. Drop the
    > return. It's just extra code that you don't need to read.
    >
    > > }
    > > return;

    >
    > Why return here? There's nothing else that happens after this. Drop the
    > return. It's just extra code that you don't need to read.
    >
    >
    >
    >
    >
    > > }
    > > else
    > > {
    > > do_look( ch, "" );
    > > return;
    > > }
    > > return;
    > > }

    >
    > > void character_data::printf( char *argument, ... )
    > > {
    > > DESCRIPTOR_DATA *descriptor;
    > > char buf[MAX_STRING_LENGTH];
    > > va_list arguments;

    >
    > > if( !argument || argument == NULL || argument[0] == '\0' )
    > > {

    >
    > Same as above. Why specify both tests when they're testing the same
    > thing? And why not flip the boolean test around, then you don't need an
    > else clause.
    >
    >
    >
    >
    >
    > > return;
    > > }
    > > else
    > > {
    > > va_start( arguments, argument );
    > > vsprintf( buf, argument, arguments );
    > > va_end( arguments );

    >
    > > if(( descriptor = this->descriptor ) == NULL )
    > > {
    > > return;
    > > }
    > > else
    > > {
    > > descriptor->printf( buf );
    > > return;
    > > }

    >
    > Why not flip the test around, and why bother assigning it to a variable?
    > And why did you choose to use the same name as what appears to be a
    > member variable? Why not:
    >
    > if (descriptor != NULL)
    > {
    > descriptor->printf(buf);
    > }
    >
    > > return;
    > > }
    > > return;

    >
    > You don't need any of these return statements. They're just extra code
    > that you don't need to read.
    >
    > > }

    >
    > > I've reviewed this a couple dozen times and don't see a problem.
    > > Granted I lack a bit of experience but at about 35000 lines of code
    > > under my belt, and as many warnings or errors as you can come up with
    > > everywhere in between, I would typically see the problem here. If
    > > anyone has any suggestions as to why I'de be experiencing this sort of
    > > a problem I'de appreciate any suggestions. Perhaps a bad
    > > initialization? Perhaps something else. This is why I'm here. And if
    > > anyone has any constructive criticism for me as far as my code is
    > > related I would appreciate it. Thanks.

    >
    > I'd recommend learning more C++. Right now it appears that you're using
    > C++ as "C with funky structs that have functions attached to them".
    > You're not using some very useful things, like references, std::string,
    > and std::list.
    >
    > And all of your source filenames end in ".c". Tends to indicate that the
    > files contain C code. But since it appears to be compiling, you probably
    > are invoking the correct compiler (g++ vs. gcc). Popular convention
    > would have the source files named ".cpp'.- Hide quoted text -
    >
    > - Show quoted text -- Hide quoted text -
    >
    > - Show quoted text -- Hide quoted text -
    >
    > - Show quoted text -


    Thanks dude I appreciate it. I'll review interpret as thoroughly as I
    can, I recently made some changes to it but don't see much of a
    problem. As far as the lack of checking to see if the character_data
    is null, that was a rather silly mistake on my part to some extent - I
    really should check in this function( and similar ones ) for the
    character not being NULL, but the only real way any of these are
    accessed is through the interpret function, which does check to see if
    the character is NULL, and I don't know( granted my lack of
    experience ) any way that I can lose that information between
    interpret and the function if I dont do anything but check to see that
    the character is set. The !argument || argument == NULL ifchecks are
    redundant and I was aware of that when I put them in, but seeing how
    in some areas it's still managing to slip by that, which doesnt make
    much sense to me, but at any rate - As far as the beyond excessive use
    of return and continue statements, I put them in just in the event
    that I opt to put something in where those statements are at. Also I
    like to keep my code as uniform as possible, and seeing how I'm self
    taught it is kind of a habit that I taught myself, but thank you for
    the pointer, I'll lighten up on them a bit. As far as the scope that
    I'm dealing with most of my variables in, I do suppose that that is a
    lack of experience but I'm often not sure how many times in a function
    I am going to have to reference any given variable, but I'll take what
    you said into account and try to minimize my scope as well as
    possible. Getting down to the fact that this is rather C-esque, this
    is largely in part due to the fact that I am self taught and the
    majority of what I have learned has in fact been in C. I'm not
    particularly familiar with the conventional built in list system so I
    try to avoid it, I'm sure that I could pull it off but I prefer not to
    delve into something that I'm not incredibly familiar with, thus the
    reason behind me not incorperating more C++ operations. I do thank you
    for the information, and the criticism, you did a fairly good job at
    it. Heh. I'll run back through interpret and see if I cant find the
    problem area. Much appreciated. Thank you.
     
    A. Anderson, Jun 28, 2007
    #4
  5. A. Anderson

    Andre Kostur Guest

    "A. Anderson" <> wrote in
    news::

    [large snip, many criticisms of the original code]

    > Thanks dude I appreciate it. I'll review interpret as thoroughly as I


    You're welcome :)

    > can, I recently made some changes to it but don't see much of a
    > problem. As far as the lack of checking to see if the character_data
    > is null, that was a rather silly mistake on my part to some extent - I
    > really should check in this function( and similar ones ) for the
    > character not being NULL, but the only real way any of these are
    > accessed is through the interpret function, which does check to see if
    > the character is NULL, and I don't know( granted my lack of
    > experience ) any way that I can lose that information between
    > interpret and the function if I dont do anything but check to see that
    > the character is set.


    If interpret checks for NULL, then you could make all of the functions
    that it calls take references instead of pointers. That way you can
    dereference the pointer back in interpret, and everybody else you call
    will work with references, which cannot be NULL, thus you don't have to
    check for them, and won't have those checks unnecessarily complicating
    your code. (Since you're coming from C, you may not be familiar with
    references.... look them up.)

    > The !argument || argument == NULL ifchecks are
    > redundant and I was aware of that when I put them in, but seeing how
    > in some areas it's still managing to slip by that, which doesnt make
    > much sense to me, but at any rate - As far as the beyond excessive use
    > of return and continue statements, I put them in just in the event
    > that I opt to put something in where those statements are at.


    The only case where I put extra stuff just in case I add something later
    is always using braces ({}) around the bodies of if, while, etc., even
    for single-line bodies. Other than that, more readable code is
    preferable. If you're continuing and/or returning from all sorts of
    places just because, you'll just end up confusing yourself (and/or
    anybody else who needs to read your code).

    > Also I
    > like to keep my code as uniform as possible, and seeing how I'm self
    > taught it is kind of a habit that I taught myself, but thank you for
    > the pointer, I'll lighten up on them a bit. As far as the scope that
    > I'm dealing with most of my variables in, I do suppose that that is a
    > lack of experience but I'm often not sure how many times in a function
    > I am going to have to reference any given variable, but I'll take what
    > you said into account and try to minimize my scope as well as
    > possible.


    Same deal. If you keep your variables to the minimum scope possible,
    then when someone is reading the code, they won't have to worry about
    what that variable may be doing at the beginning of the function... it
    won't exist there. It will only exist where needed. And later on when
    your objects may do non-trivial amounts of work in the constructor,
    delaying their construction to when they're actually used may save some
    execution time in the cases where that code block isn't executed.

    > Getting down to the fact that this is rather C-esque, this
    > is largely in part due to the fact that I am self taught and the
    > majority of what I have learned has in fact been in C.


    Most of us have been there... it takes some time to use all of the
    benefits of C++.

    > I'm not
    > particularly familiar with the conventional built in list system so I
    > try to avoid it, I'm sure that I could pull it off but I prefer not to
    > delve into something that I'm not incredibly familiar with, thus the
    > reason behind me not incorperating more C++ operations.


    Oh... get familiar with it. It's been tested & debugged far more than
    your own code :) You _really_ want to get familiar with the Standard
    Library.

    > I do thank you
    > for the information, and the criticism, you did a fairly good job at
    > it. Heh. I'll run back through interpret and see if I cant find the
    > problem area. Much appreciated. Thank you.


    You're welcome.
     
    Andre Kostur, Jun 28, 2007
    #5
  6. A. Anderson

    A. Anderson Guest

    On Jun 28, 11:56 am, Andre Kostur <> wrote:
    > "A. Anderson" <> wrote innews::
    >
    > [large snip, many criticisms of the original code]
    >
    > > Thanks dude I appreciate it. I'll review interpret as thoroughly as I

    >
    > You're welcome :)
    >
    > > can, I recently made some changes to it but don't see much of a
    > > problem. As far as the lack of checking to see if the character_data
    > > is null, that was a rather silly mistake on my part to some extent - I
    > > really should check in this function( and similar ones ) for the
    > > character not being NULL, but the only real way any of these are
    > > accessed is through the interpret function, which does check to see if
    > > the character is NULL, and I don't know( granted my lack of
    > > experience ) any way that I can lose that information between
    > > interpret and the function if I dont do anything but check to see that
    > > the character is set.

    >
    > If interpret checks for NULL, then you could make all of the functions
    > that it calls take references instead of pointers. That way you can
    > dereference the pointer back in interpret, and everybody else you call
    > will work with references, which cannot be NULL, thus you don't have to
    > check for them, and won't have those checks unnecessarily complicating
    > your code. (Since you're coming from C, you may not be familiar with
    > references.... look them up.)
    >
    > > The !argument || argument == NULL ifchecks are
    > > redundant and I was aware of that when I put them in, but seeing how
    > > in some areas it's still managing to slip by that, which doesnt make
    > > much sense to me, but at any rate - As far as the beyond excessive use
    > > of return and continue statements, I put them in just in the event
    > > that I opt to put something in where those statements are at.

    >
    > The only case where I put extra stuff just in case I add something later
    > is always using braces ({}) around the bodies of if, while, etc., even
    > for single-line bodies. Other than that, more readable code is
    > preferable. If you're continuing and/or returning from all sorts of
    > places just because, you'll just end up confusing yourself (and/or
    > anybody else who needs to read your code).
    >
    > > Also I
    > > like to keep my code as uniform as possible, and seeing how I'm self
    > > taught it is kind of a habit that I taught myself, but thank you for
    > > the pointer, I'll lighten up on them a bit. As far as the scope that
    > > I'm dealing with most of my variables in, I do suppose that that is a
    > > lack of experience but I'm often not sure how many times in a function
    > > I am going to have to reference any given variable, but I'll take what
    > > you said into account and try to minimize my scope as well as
    > > possible.

    >
    > Same deal. If you keep your variables to the minimum scope possible,
    > then when someone is reading the code, they won't have to worry about
    > what that variable may be doing at the beginning of the function... it
    > won't exist there. It will only exist where needed. And later on when
    > your objects may do non-trivial amounts of work in the constructor,
    > delaying their construction to when they're actually used may save some
    > execution time in the cases where that code block isn't executed.
    >
    > > Getting down to the fact that this is rather C-esque, this
    > > is largely in part due to the fact that I am self taught and the
    > > majority of what I have learned has in fact been in C.

    >
    > Most of us have been there... it takes some time to use all of the
    > benefits of C++.
    >
    > > I'm not
    > > particularly familiar with the conventional built in list system so I
    > > try to avoid it, I'm sure that I could pull it off but I prefer not to
    > > delve into something that I'm not incredibly familiar with, thus the
    > > reason behind me not incorperating more C++ operations.

    >
    > Oh... get familiar with it. It's been tested & debugged far more than
    > your own code :) You _really_ want to get familiar with the Standard
    > Library.
    >
    > > I do thank you
    > > for the information, and the criticism, you did a fairly good job at
    > > it. Heh. I'll run back through interpret and see if I cant find the
    > > problem area. Much appreciated. Thank you.

    >
    > You're welcome.


    So... I hate to be an absolute bother and I must apologize sincerely,
    however Im at a point that SEEMS to be beyond my knowledge, and I feel
    like an absolute noobskate, but I'll proceed with my problem none the
    less. I've gone through character_data::printf and
    descriptor_data::interpret, and I can't seem to find the problem.
    Check out this stack:

    (gdb) bt
    #0 0xb7ce72a3 in strlen () from /lib/tls/i686/cmov/libc.so.6
    #1 0xb7cbb2f7 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
    #2 0xb7cd341b in vsprintf () from /lib/tls/i686/cmov/libc.so.6
    #3 0x08049ba4 in character_data::printf (this=0x800, argument=0x0)
    at character.c:193
    #4 0x0804a956 in do_look (ch=0x800, argument=0x805bd64 "") at
    command.c:1062
    #5 0x0804ce33 in descriptor_data::interpret (this=0xbff35e6c,
    argument=0x805bd60 "look") at descriptor.c:268
    #6 0x0804d903 in descriptor_data::process_input (this=0x8656e30)
    at descriptor.c:207
    #7 0x0804bc08 in main_loop () at core.c:132
    #8 0x0804c080 in main (argc=2, argv=0xbff3a0c4) at core.c:34
    (gdb) select 5
    Current language: auto; currently c++
    (gdb) print this->character
    $1 = (CHARACTER_DATA *) 0xbff35f40
    (gdb) print this->character->name
    $2 = 0x6e616d6d <Address 0x6e616d6d out of bounds>

    The descriptors character is not null, but I can't access the name.
    This strikes me as a bit odd and I assume that the problem is due to
    poor allocation of the data structure to memory. As it stands I've got
    no alternative that I know of other than creating a new
    character_data, and then linking it into my list. My list start and
    finish are both initialized to null and set to be a character when I
    load all of the characters out of my SQL database, but apparently I
    went about something wrong somewhere. Any suggestions from anyone
    would be appreciated. Thank you again, and if you need any
    information( functions etc ) I am more than happy to share them if
    it's gonna get me a step or two closer to wrapping this bird up.
     
    A. Anderson, Jun 28, 2007
    #6
  7. A. Anderson

    Kevin Handy Guest

    A. Anderson wrote:
    > Howdy everyone,
    >
    > I'm experiencing a problem with a program that I'm developing. Take a
    > look at this stack report from GDB -
    >
    > #0 0xb7d782a3 in strlen () from /lib/tls/i686/cmov/libc.so.6
    > #1 0xb7d4c2f7 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
    > #2 0xb7d6441b in vsprintf () from /lib/tls/i686/cmov/libc.so.6
    > #3 0x08049ba0 in character_data::printf (this=0x800, argument=0x0)
    > at character.c:198


    "this=0x800" doesn't sound right to me.
    0x800 is too small and neat compared with all the other 'this's.

    I suspect something is wrong with the pointer you are using
    for this class.

    > #4 0x0804a956 in do_look (ch=0x800, argument=0x8659164 "") at
    > command.c:1062


    where does this "ch=0x800" come from? Suspiciously
    like the this=0x800 value.

    > #5 0x0804ce7d in descriptor_data::interpret (this=0x8656e30,
    > argument=0x805bde0 "look") at descriptor.c:278


    See, here "this" is far distant from 0x800.
    I'd expect the memory used to be closer together.
    Could, however, depend on how the two were allocated.

    > #6 0x0804d945 in descriptor_data::process_input (this=0x8656e30)
    > at descriptor.c:207
    > #7 0x0804bc08 in main_loop () at core.c:132
    > #8 0x0804c080 in main (argc=2, argv=0xbf8cd4c4) at core.c:34
    >
    > And here are the relevant functions:
    >
    > void do_look( CHARACTER_DATA *ch, char *argument )
    > {
    > ROOM_DATA *room;
    > CHARACTER_DATA *ich;
    >
    > if( !argument || argument == NULL || argument[0] == '\0' )
    > {
    > if(( room = ch->in_room ) == NULL )
    > {
    > ch->printf( "It doesnt seem that you are in a room. Please contact
    > an administrator.\n\r" );
    > return;
    > }
    > else
    > {
    > ch->printf( "%s\n\r", room->name );
    > ch->printf( "-------------------------\n\r" );
    > if( !room->description || room->description[0] == '\0' )
    > {
    > }
    > else
    > {
    > ch->printf( "%s\n\r", room->description );
    > ch->printf( "-------------------------\n\r" );
    > }
    > for( ich = room->first_character; ich; ich = ich->next )
    > {
    > if( ich != ch && ich->descriptor && ich->descriptor != NULL && ich-
    >> descriptor->connected == CON_PLAYING )

    > {
    > ch->printf( "%s\n\r", ich->long_desc( ));
    > }
    > else
    > {
    > continue;
    > }
    > continue;
    > }
    > return;
    > }
    > return;
    > }
    > else
    > {
    > do_look( ch, "" );
    > return;
    > }
    > return;
    > }
    >
    > void character_data::printf( char *argument, ... )


    is it "CHARACTER_DATA" as in the above function,
    or "character_data" as defined here? C++ is case
    sensitive. Or are you trying to play unnecessary
    "C" games using typedef, and getting confused?

    > {
    > DESCRIPTOR_DATA *descriptor;
    > char buf[MAX_STRING_LENGTH];
    > va_list arguments;
    >
    > if( !argument || argument == NULL || argument[0] == '\0' )
    > {
    > return;
    > }
    > else
    > {
    > va_start( arguments, argument );
    > vsprintf( buf, argument, arguments );
    > va_end( arguments );
    >
    > if(( descriptor = this->descriptor ) == NULL )
    > {
    > return;
    > }
    > else
    > {
    > descriptor->printf( buf );
    > return;
    > }
    > return;
    > }
    > return;
    > }
    >
    > I've reviewed this a couple dozen times and don't see a problem.
    > Granted I lack a bit of experience but at about 35000 lines of code
    > under my belt, and as many warnings or errors as you can come up with
    > everywhere in between, I would typically see the problem here. If
    > anyone has any suggestions as to why I'de be experiencing this sort of
    > a problem I'de appreciate any suggestions. Perhaps a bad
    > initialization? Perhaps something else. This is why I'm here. And if
    > anyone has any constructive criticism for me as far as my code is
    > related I would appreciate it. Thanks.
    >


    ----== Posted via Newsfeeds.Com - Unlimited-Unrestricted-Secure Usenet News==----
    http://www.newsfeeds.com The #1 Newsgroup Service in the World! 120,000+ Newsgroups
    ----= East and West-Coast Server Farms - Total Privacy via Encryption =----
     
    Kevin Handy, Jun 28, 2007
    #7
  8. A. Anderson

    A. Anderson Guest

    On Jun 28, 1:06 pm, Kevin Handy <> wrote:
    > A. Anderson wrote:
    > > Howdy everyone,

    >
    > > I'm experiencing a problem with a program that I'm developing. Take a
    > > look at this stack report from GDB -

    >
    > > #0 0xb7d782a3 in strlen () from /lib/tls/i686/cmov/libc.so.6
    > > #1 0xb7d4c2f7 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
    > > #2 0xb7d6441b in vsprintf () from /lib/tls/i686/cmov/libc.so.6
    > > #3 0x08049ba0 in character_data::printf (this=0x800, argument=0x0)
    > > at character.c:198

    >
    > "this=0x800" doesn't sound right to me.
    > 0x800 is too small and neat compared with all the other 'this's.
    >
    > I suspect something is wrong with the pointer you are using
    > for this class.
    >
    > > #4 0x0804a956 in do_look (ch=0x800, argument=0x8659164 "") at
    > > command.c:1062

    >
    > where does this "ch=0x800" come from? Suspiciously
    > like the this=0x800 value.
    >
    > > #5 0x0804ce7d in descriptor_data::interpret (this=0x8656e30,
    > > argument=0x805bde0 "look") at descriptor.c:278

    >
    > See, here "this" is far distant from 0x800.
    > I'd expect the memory used to be closer together.
    > Could, however, depend on how the two were allocated.
    >
    >
    >
    >
    >
    > > #6 0x0804d945 in descriptor_data::process_input (this=0x8656e30)
    > > at descriptor.c:207
    > > #7 0x0804bc08 in main_loop () at core.c:132
    > > #8 0x0804c080 in main (argc=2, argv=0xbf8cd4c4) at core.c:34

    >
    > > And here are the relevant functions:

    >
    > > void do_look( CHARACTER_DATA *ch, char *argument )
    > > {
    > > ROOM_DATA *room;
    > > CHARACTER_DATA *ich;

    >
    > > if( !argument || argument == NULL || argument[0] == '\0' )
    > > {
    > > if(( room = ch->in_room ) == NULL )
    > > {
    > > ch->printf( "It doesnt seem that you are in a room. Please contact
    > > an administrator.\n\r" );
    > > return;
    > > }
    > > else
    > > {
    > > ch->printf( "%s\n\r", room->name );
    > > ch->printf( "-------------------------\n\r" );
    > > if( !room->description || room->description[0] == '\0' )
    > > {
    > > }
    > > else
    > > {
    > > ch->printf( "%s\n\r", room->description );
    > > ch->printf( "-------------------------\n\r" );
    > > }
    > > for( ich = room->first_character; ich; ich = ich->next )
    > > {
    > > if( ich != ch && ich->descriptor && ich->descriptor != NULL && ich-
    > >> descriptor->connected == CON_PLAYING )

    > > {
    > > ch->printf( "%s\n\r", ich->long_desc( ));
    > > }
    > > else
    > > {
    > > continue;
    > > }
    > > continue;
    > > }
    > > return;
    > > }
    > > return;
    > > }
    > > else
    > > {
    > > do_look( ch, "" );
    > > return;
    > > }
    > > return;
    > > }

    >
    > > void character_data::printf( char *argument, ... )

    >
    > is it "CHARACTER_DATA" as in the above function,
    > or "character_data" as defined here? C++ is case
    > sensitive. Or are you trying to play unnecessary
    > "C" games using typedef, and getting confused?
    >
    >
    >
    >
    >
    > > {
    > > DESCRIPTOR_DATA *descriptor;
    > > char buf[MAX_STRING_LENGTH];
    > > va_list arguments;

    >
    > > if( !argument || argument == NULL || argument[0] == '\0' )
    > > {
    > > return;
    > > }
    > > else
    > > {
    > > va_start( arguments, argument );
    > > vsprintf( buf, argument, arguments );
    > > va_end( arguments );

    >
    > > if(( descriptor = this->descriptor ) == NULL )
    > > {
    > > return;
    > > }
    > > else
    > > {
    > > descriptor->printf( buf );
    > > return;
    > > }
    > > return;
    > > }
    > > return;
    > > }

    >
    > > I've reviewed this a couple dozen times and don't see a problem.
    > > Granted I lack a bit of experience but at about 35000 lines of code
    > > under my belt, and as many warnings or errors as you can come up with
    > > everywhere in between, I would typically see the problem here. If
    > > anyone has any suggestions as to why I'de be experiencing this sort of
    > > a problem I'de appreciate any suggestions. Perhaps a bad
    > > initialization? Perhaps something else. This is why I'm here. And if
    > > anyone has any constructive criticism for me as far as my code is
    > > related I would appreciate it. Thanks.

    >
    > ----== Posted via Newsfeeds.Com - Unlimited-Unrestricted-Secure Usenet News==----http://www.newsfeeds.comThe #1 Newsgroup Service in the World! 120,000+ Newsgroups
    > ----= East and West-Coast Server Farms - Total Privacy via Encryption =----- Hide quoted text -
    >
    > - Show quoted text -- Hide quoted text -
    >
    > - Show quoted text -


    Sadly, I am using the needlessly confusing typedef setup. I can't
    agree more that the difference in memory locations is obscure at the
    very least. The ch in do_look is being given to it by
    descriptor_data::interpret. Just for reference here is interpret:

    void descriptor_data::interpret( char *argument, ... )
    {
    COMMAND_DATA *command;
    char buf[MAX_STRING_LENGTH];
    char arg1[MAX_STRING_LENGTH];
    va_list arguments;

    if( !argument || argument[0] == '\0' )
    {
    log_debug( "descriptor_data::connect_name: null argument." );
    return;
    }
    else
    {
    va_start( arguments, argument );
    vsprintf( buf, argument, arguments );
    va_end( arguments );

    if( this->character == NULL )
    {
    this->printf( "There seems to have been a problem with your
    connection, please re-enter your name:\n\r" );
    this->connected = CON_CONNECT_NAME;
    return;
    }
    else
    {
    argument = one_argument( argument, arg1 );

    if(( command = get_command( arg1 )) == NULL || command->function ==
    NULL )
    {
    this->printf( "Huh?\n\r" );
    this->character->printf( this->character->print_prompt( ));
    return;
    }
    else if( command->immortal > this->character->immortal )
    {
    this->character->printf( "Huh?\n\r" );
    this->character->printf( this->character->print_prompt( ));
    return;
    }
    else
    {
    log_debug( "Passing '%s' to command %s.", argument, command-
    >name );

    ( command->function )( this->character, argument );
    this->character->printf( this->character->print_prompt( ));
    return;
    }
    return;
    }
    return;
    }
    return;
    }
     
    A. Anderson, Jun 28, 2007
    #8
  9. A. Anderson wrote:
    > Sadly, I am using the needlessly confusing typedef setup. I can't
    > agree more that the difference in memory locations is obscure at the
    > very least. The ch in do_look is being given to it by
    > descriptor_data::interpret. Just for reference here is interpret:


    Please read this:
    http://www.netmeister.org/news/learn2quote.html

    > void descriptor_data::interpret( char *argument, ... )
    > {

    [...code...]
    > else
    > {
    > log_debug( "Passing '%s' to command %s.", argument, command-
    >> name );

    > ( command->function )( this->character, argument );


    I guess this calls do_look(). Since interpret() doesn't change
    this->character, the variable has to be wrong earlier. Try to put some
    printf or log_debug statements to see where this variable gets this wrong
    value. Start by looking at the first assignment, maybe it gets its wrong
    value already there.

    > this->character->printf( this->character->print_prompt( ));
    > return;
    > }

    [...code...]

    --
    Thomas
    http://www.netmeister.org/news/learn2quote.html
     
    Thomas J. Gritzan, Jun 28, 2007
    #9
    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:
    5
    Views:
    26,932
    Mike Schilling
    Mar 29, 2006
  2. guenther schoebel
    Replies:
    2
    Views:
    6,517
    Eric Cartman
    Oct 10, 2003
  3. Bhushit Joshipura

    defaulting argument to previous argument

    Bhushit Joshipura, Dec 29, 2003, in forum: C++
    Replies:
    5
    Views:
    419
  4. dutone
    Replies:
    8
    Views:
    110
    dutone
    Jul 2, 2004
  5. eli m
    Replies:
    30
    Views:
    351
    Frank Millman
    Jan 25, 2013
Loading...

Share This Page