[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Some C++ programming guidelines
From:       Armen Nakashian <armen () yawara ! anime ! net>
Date:       1998-09-13 17:05:09
[Download RAW message or body]

Recently I compiled KDE on a platform with an extremely strict C++
compiler (DEC CXX v6.0).  It didn't get very far before I had to go into
the code and fix dozens of minor ANSI C++ violations that GNU G++ doesn't
even wink at.  I've put together a list of the most common ones with some
notes.

Note: I don't gaurantee that my analsies are correct, so you can't stone
me if you take issue with anything you see.

- Armen Nakashian
"Email is the next best thing to a female"
http://yawara.anime.net/~armen/



Bad things to do in C++ that makes your code broken


1) Know when to use const.  

   Here's a line of bad code:
   const QString blah(const QString s);

   It's wrong in two respects; you cannot put const in front of non-pointer
   types.  Both the parameter and the return value are copies of the
   original information.  If you want to change them, go ahead, they're
   your copies.  No one else has pointers to them.
   
   This is perfectly reasonable though:
   const QString* blah(const QString* s);

   This means both the return value cannot be changed once a pointer to
   it is taken, and that this function will not be altering the value of
   the input parameter s.

   
   Here's the third use of const, the one that I suspect most people are
   looking for:
   QString blah(QString s) const;

   This means that method blah takes a parameter s (that might change, but
   we don't care), returns a QString (that can change, no one cares),
   but during its execution, blah will absolutely not alter the value
   of the object it is a method of.  That is:

   Class c {
        public:
	QString blah(QString s) const;
        
        QString someText;
   };


   By running c::blah (or ObjectOfTypeC->blah("hello") ) we are garanteed
   that the value of c::someText will not change.  Why do we need this?  
   Its for when you have const* pointers to an object.  It lets the compiler
   _really_ know what methods you can call, and not violate the 'constness'
   of the pointer.




2) Default Function Parameters Only Go in Headers
 
   This has to be by far the most common C++ syntax violation.
   When writing a function that takes default parameters, you have to 
   understand that it is the PROTOTYPE of the function that gives the 
   compiler the default values, not the piece of code itself.

   class Exchange {
      float convertDollars(int dollars, Unit u=YEN);
   };

   float Exchange::convertDollars(int dollars,Unit u=YEN) {
	.
	.
	.
   }

   Notice how 'Unit u=YEN' is duplicated.  This is WRONG.  The default
   parameter should only be mentioned in the class definition.  The corrent
   implementation of Exchange::convertDollars is:

   float Exchange::convertDollars(int dollars,Unit u) {
	.
	.
   }

   See, your compiler picks up the default values from the header files,
   and when it detects that you've "left out" a parameter, it substitutes
   whatever value was written in the header file.  The actual method
   implementation has nothing to do with default parameters.

   Note:  If you're implementing a method within a class definition, its
   perfectly fine to put the default parameters there.  
   
   class Exchange {
	float convertDollars(int dollars,Unit u=YEN) {
	    .
	    .
	}
   };

   This is perfectly fine.




3) Do not put Scope Resolvers on method declarations (or implementations)
   within a class definition

   Basically, don't do this:

   class Exchange {
	float Exchange::blah(int dollars, Unit u=YEN);
              ^^^^^^^^^^
   };

   That Exchange:: right there causes a problem with strict compilers.
   They don't like it, you don't need it, so don't do it.

   Extra Notes:
   There are times where you need the Classname:: prefix on methods though,
   but NEVER for declaring it.  You use the prefix within method bodies when
   referencing a static member of a class (attribute or method), or when you
   want to call a specific method from a base class, especially when 
   multiple inheritence masks the name of methods in the base classes.


4) Overriden Methods Must have Signatures Identical to the Base Class

   This one came up a bunch of times.  Often you will inherit from a
   class (say QWidget) and re-implement several of its classes.  It is
   critical that you declare the overriding method with EXACTLY the same
   signature as the method in the base class.

   class QWidget {
	QSize sizeHint(void) const;
        void setName(const char* name);
   };

   class QGramaphone : public QWidget {
	
	QSize sizeHint(void);
   };

   QGramaphone::sizeHint overrides QWidget::sizeHint, right?  Wrong.
   Notice the const-ness differs.  This is enough to confuse the compiler
   and cause it to select the wrong version of sizeHint (if its smart enough;
   an oxymoron at best).  


   Here's another one:

   class QKaliedascope : public QWidget {
	
	void setName(char* name);
   }

   This one is wrong because the original setName() took a CONST char*.
   This one is different, it just takes a char*.


   
   Basically, make sure your declaration matches EXACTLY the one in the
   base class.  sizeHint() in particular was one a lot of people missed.




5) Do Not Pass Pointers to Temporaries

   This is extremely wrong:

   void printAQString(QString* s)  { cout << *s } 

   printAQString(&QString("Hello"));

   Whats going on here?  We're constructing a new QString (with the text
   "hello") and then passing its pointer to function printAQString,
   which just prints the string.  Seems harmless.

   But its illegal.  You can't pass pointers to termporaries like that. 
   A temporary is an object constructed within a function call, or because
   of a conversion, or because of a return statement.  These are objects
   that are GAURANTEED to be destructed immediately after a call is
   completed.  For this reason, its innappropriate to take their pointers.


   Consider this:

   QString* nameptr;

   void setGlobalName(QString* s) {
	nameptr=s;
   }


   setGlobalName(&QString("Hello"));
   cout << *nameptr;   


   When were just printing the temporary, we didn't care if it got
   destructed after we returned from our function call.  Here, we
   get a nice segfault.  Why?  nameptr points at the temporary, which
   is destructed immediately after setGlobalName() finishes.  The next
   line tries to dereference a free'd memory location.  Bad.


   Whats the RIGHT way to pass a pointer?  Simple:

   QString obj("Hello");
   setGlobalName(&obj);

   // Now you can do this, but only within the scope of the block
   // obj was declared in.
   cout << *nameptr;


   Its more work, but its the right thing to do.  obj will be
   destructed after we leave the block that the code is executing in,
   so you might still run into NULL pointer problems, but atleast
   things are more obvious in this case (and your code will compile on
   a strict compiler).




6) Don't Return const Non-pointers from Methods

   This one is subtle, and not horribly important, but I think its
   worth mentioning.

   class Exchange {
	 const float convertDollars(int dollars,Unit u);
   };


   Whats wrong here?  Returning the const float.  When this function
   returns, some block of code is waiting to take it, right?

   const float f=convertDollars(100,YEN);

   f is going to be a new float, with no connection to any other
   floats declared anywhere at anytime.  Its also a const, so its
   value can't be changed.  But convertDollar didn't have to declare
   that its returning a const float to make this possible; It's the
   call (and declaration-initialization, which must happen at the same
   time BTW) that determine the const-ness of f.

   By declaring that convertDollars returns const float, we can't ever
   do this (without extra work):

   float f=convertDollars(a,YEN);
   float g=f+convertDolloars(b,EURO);


   Non-pointer and non-reference objects are COPIES of whatever values
   they are initialized too.  Return values (and paramters) involving
   them don't need to be const; if they do change, the changes will be
   local only to the called method.

				   

[prev in list] [next in list] [prev in thread] [next in thread]