Path: utzoo!utgpu!jarvis.csri.toronto.edu!clyde.concordia.ca!uunet!cs.utexas.edu!tut.cis.ohio-state.edu!ucbvax!van-bc!mdavcr!rdr
From: rdr@mdavcr.UUCP (Randolph Roesler)
Newsgroups: comp.lang.c++
Subject: lint++ -> really "AutoCodeReview" -- Ideas requested.
Summary: Ideas for an expert codereviewer wanted.
Keywords: C++ lint code review
Message-ID: <695@acrux.mdavcr.UUCP>
Date: 8 Dec 89 03:41:54 GMT
Reply-To: rdr@mdavcr.UUCP (Randolph Roesler)
Organization: MacDonald Dettwiler, Richmond, B.C., Canada
Lines: 151

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