rdr@mdavcr.UUCP (Randolph Roesler) (12/08/89)
We all know that C++ is a better C, with OO and prototypes and
better type matching and etc.
What we also know is that C++ is complex, nearly as complex as ADA.
All those rules about operator new (which really should be called
operator allocate), operator = (assignment), copy constructors,
protected, private, public, ...... 10 Million and One rules to learn.
And some of these rules are very subtle, like this one I discovered
the other day (which I will detail shortly)
In the old days, we KNEW what was going on, and in which order things
happened. Maybe, if good garbage collection was around (and cheap and
efficient), we could all stop worring about these issues. But for now,
C++ is the best we have so WE better improve it (or retire).
We had lint in the old days to catch things like
if ( x = 0 ) then
something_imporant();
which is almost certainly an error.
What we need is lint++ -- an expert code reviewer which can catch problems
like that above, and the more subtle problems, like that detailed
below.
I would like to collect a list of lint-like heuristics for C++, things that
the compiler cannot be expected to catch, but which are:
o likely program bugs
o unsafe coding practices
o portablity issues
o etc.
Now is the time to collect these types of rules, while we are all
learning. It is now that we will be discovering creative new ways
shoot ourselves in the foot (feet if you are a good shot :)).
Please e-mail in your contributions and I will summarize to the net.
Contributions that express a rule like:
Constructor(s) with dynamic memory allocation without assigment
operator(s) might cause heap problems !! (see sample problem below)
and a more mundane one:
Assignment within condition expression of an if statement might
be mistype of ==.
are preferred, but I am not greedy, give what you can. Heuristics which
address mainly theological points of style are too abstract -- let's avoid
those.
Now, to start things off, here is my very subtle problem which
started this whole train of thought. Can you spot the error ?
#include <string.h>
#include "ComIdent.hh"
ComIdent::ComIdent()
{
string = 0;
}
ComIdent::ComIdent( const char * const token )
{
if ( token )
{ string = new char[strlen(token)+1];
strcpy( string,token );
}
else
string = 0;
}
ComIdent::ComIdent( const ComIdent & token )
{
if ( token.string )
{ string = new char[strlen(token.string)+1];
strcpy( string,token.string );
}
else
string = 0;
}
ComIdent::~ComIdent()
{
if ( string )
delete[strlen(string)+1] string;
}
Did you see the problem? I did not --- it took me 19 hours or so
to find out what was going on. I eventually had to look at the C++ 2.0
produced C code to figure it out. Here is the problem:
Statements like
ComIndent ident_new = ident_old;
fail because
a) Copy constructor is called to copy ident_old into a temp
ComIdent object. Memory is malloc'ed (by new) OK.
(I made the copy constructor to avoid the problem you are
about to discover; unfortunately, I did not understand that
C++ creates temps on initialization. Too bad.)
b) Default memberwise copy is performed from temp into ident_new.
Pointer to character array is member of ComIdent types, and is
copied faithfully. (I had thought the copy constructor would
be used here.)
c) Destructor is called to free memory of temp. Newly allocated
string is freed.
d) Next new overwrites data area of string (which is pointed to by
the now invalid ident_new).
So simple ? Everyone knows that ! (So do I.) Well, it so happens
that indent_new was passed to another constructor as the first argument
several function calls later. What I saw was that C++ was making an
allocation error because the pointer value of this was the same as
the constructor's first value. Not only that, but the program did not crash,
instead it just printed "(null)" all over the place.
The fix --- delete the copy constructor and replace with an assignment
one.
ComIdent & ComIdent::operator = ( const ComIndent & token )
{
if ( string )
delete[strlen(string)+1] string;
if ( token.string )
{
string = new char[strlen(token.string)+1];
strcpy( string,token.string );
}
else
string = 0;
}
The point --- if I had read about this problem (or had lint++) this might
not have cost me so much time. So emails away !
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It's not the size of your signature that Randy Roesler
counts - it's how you use it! MacDonald Dettwiler & Assc.
email ...!uunet!van-bc!mdavcr!rdr BC Canada 604-278-3411kenny@hpcuhc.HP.COM (Kenneth Lee) (12/09/89)
/ hpcuhc:comp.lang.c++ / rdr@mdavcr.UUCP (Randolph Roesler) / 7:41 pm Dec 7, 1989 / > > We had lint in the old days to catch things like > > if ( x = 0 ) then > something_imporant(); > > which is almost certainly an error. I know it's certainly an error, but what are you trying to point out? Are you trying to say lint should do one of the following: 1. tell you a syntax error if ( expression ) then statement where then is not a keyword in C. 2. tell you a logic error if ( something which is always false ) thisWillNeverGetExecute(); Are these the jobs of lint? kenny
Roy.Browning@f506.n106.z1.fidonet.org (Roy Browning) (12/11/89)
> ComIdent::~ComIdent() > { > if ( string ) > delete[strlen(string)+1] string; > } > The fix --- delete the copy constructor and replace with an assignment > one. Randolph: I have sucessfully used both a copy constructor and an overloading assignment operator in a test class. However from my limited reading on C++ I believe your problem is your destructor. From reading Lippman pages 240 & 241 you are deleting an array of pointers stored in "string" where "[x]" is the number to "free()". From the source code I've seen all "delete" does is to call "free()" and pass it a "char *". Telling "delete" there are "[X]" pointers stored at location "string" should cause some VERY unusual problems if I'm correct. I could use a lint myself, Roy Browning
jeenglis@nunki.usc.edu (Joe English) (12/13/89)
Roy.Browning@f506.n106.z1.fidonet.org (Roy Browning) writes: > > > ComIdent::~ComIdent() > > { > > if ( string ) > > delete[strlen(string)+1] string; > > } > I believe your problem is your destructor. From reading Lippman >you are deleting an array of pointers stored in "string" where "[x]" is the >number to "free()". From the source code I've seen all "delete" does is to >call "free()" and pass it a "char *". Telling "delete" there are "[X]" >pointers stored at location "string" should cause some VERY unusual problems >if I'm correct. Actually, delete[X] p; is used when p points at an array of X objects with destructors. It calls the destructor on each of the X objects but only passes "p" to free() (or the equivalent function.) Since 'char' doesn't have a destructor, the above code is equivalent to just 'delete string;' --Joe English jeenglis@nunki.usc.edu
rdr@mdavcr.UUCP (Randolph Roesler) (12/13/89)
In article <590001@hpcuhc.HP.COM> kenny@hpcuhc.HP.COM (Kenneth Lee) writes: >/ hpcuhc:comp.lang.c++ / rdr@mdavcr.UUCP (Randolph Roesler) / 7:41 pm Dec 7, 1989 / >> >> We had lint in the old days to catch things like >> >> if ( x = 0 ) then ^^^^ >> something_imporant(); >> >> which is almost certainly an error. > >I know it's certainly an error, but what are you trying to point out? > Yes -- I made a typing error -- I typed what I say, rather than what C++ accepts. Catching this error is the compilers job. The possible error of the assignment rather than a comparison would be a lint tool's job. What I want from you (dear net reader), is hueristic rules about situations in C++ which the compiler can not or will not warn you about. -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ It's not the size of your signature that Randy Roesler counts - it's how you use it! MacDonald Dettwiler & Assc. email ...!uunet!van-bc!mdavcr!rdr BC Canada 604-278-3411
rdr@mdavcr.UUCP (Randolph Roesler) (12/13/89)
In article <5457.25845C8E@urchin.fidonet.org> Roy.Browning@f506.n106.z1.fidonet.org (Roy Browning) writes: > > > ComIdent::~ComIdent() > > { > > if ( string ) > > delete[strlen(string)+1] string; > > } > > > The fix --- delete the copy constructor and replace with an assignment > > one. > >Randolph: > > I have sucessfully used both a copy constructor and an overloading >assignment operator in a test class. However from my limited reading on C++ I >believe your problem is your destructor. From reading Lippman pages 240 & 241 >you are deleting an array of pointers stored in "string" where "[x]" is the >number to "free()". From the source code I've seen all "delete" does is to >call "free()" and pass it a "char *". Telling "delete" there are "[X]" >pointers stored at location "string" should cause some VERY unusual problems >if I'm correct. > > I could use a lint myself, > > Roy Browning > Thanks for the reply Roy. The program is working fine now, so I do not think that the destructor is the problem. My use of delete is symmetric with may use of new. I.e. some_type * xxxx = new some_type [size]; delete [size] xxxx; In this case, some_type is `char', and thus, neither new or delete need to call constructors/destructors. The key passage in the manual is on page 21 (SUN C++ docs, section 5.3.4), which says (much left out): The form delete [ expression ] cast_expression is used to delete arrays.... destructors (if any) ... will be invoked. Still, you bring up a good point. If some_type was orginal typedefined as character, and you left out the [ expression ] construction from the delete statement, everything would probably work out fine. (I guess I should try this !). If, latter, a change my typedefine of some_type to some other type with constructors and destructors, then the code would probably start to fail. Could a tool be written to track varible usage and warn you about this type of situations ? Any more situations like this ? -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ It's not the size of your signature that Randy Roesler counts - it's how you use it! MacDonald Dettwiler & Assc. email ...!uunet!van-bc!mdavcr!rdr BC Canada 604-278-3411
Roy.Browning@f506.n106.z1.fidonet.org (Roy Browning) (12/14/89)
> > > ComIdent::~ComIdent() > > > { > > > if ( string ) > > > delete[strlen(string)+1] string; > > > } > Actually, delete[X] p; is used when p points at an > array of X objects with destructors. It calls the > destructor on each of the X objects but only passes > "p" to free() (or the equivalent function.) > > Since 'char' doesn't have a destructor, the above > code is equivalent to just 'delete string;' Joe: Ouch, guess I deserved that. Couched my answer in very loose 'C' jargon in the C++ newsgroup and got my head handed to me <grin>. I habitually use imprecise terminology relying on the overall meaning rather than the exact words. Will have to revert to a policy of only posting tested code. Spent a couple of hours proving that the code below would loose random pointers to free(). Even modified my library to to demonstrate exactly what was occurring. main() { char* str = "Test string"; String* a = (String*) new String; *a = str; cout << a; delete[5] a; exit(0); } Then I looked at your reply and the original message and observed that I had only proven that I had misread the original missive. You are quite correct, "delete[x] string" calls free only once deallocating the string. I stand corrected, Roy Browning
Roy.Browning@f506.n106.z1.fidonet.org (Roy Browning) (12/14/89)
> From: rdr@mdavcr.UUCP (Randolph Roesler) > Date: 12 Dec 89 21:51:32 GMT > Organization: MacDonald Dettwiler, Richmond, B.C., Canada > Message-ID: <701@acrux.mdavcr.UUCP> > Newsgroups: comp.lang.c++ Roland: What I so badly related in the previous message is that I find the syntax listed below sufficient for deallocating character arrays. foo() { char* string = new char[number of characters]; ..... delete string; } However the syntax below is what is suggested by Lippman for an array of pointers to objects. The String destructor is invoked for each of the "size" elements of tbl2. foo() { String tbl[ size ]; String *tbl2 = new String[ size ]; ..... while (cin >> tbl[i]); .... delete [ size ] tbl2; } I mistakely assumed that your use of "new" was to return an array of object pointers rather than an array of characters, my mistake. Roy Browning