Sign in to follow this  
TimChan

Programming traps

Recommended Posts

Just wanna know the common programming mistakes which can't be detected by compilers. Here is an example on which I spent the whole yesterday to find it out.
vector<CNPC*> NPCs;

for(i = 0; i < 100; ++i) {
	CNPC npc(Vector(i, 0));	// create a line of NPCs
	NPCs.push_back(&npc);
}
//	Now the vector NPCs stores nothing as the objects are deleted just after the scope


Share this post


Link to post
Share on other sites
ifstream config_file("config.tx"); //oops that should be "config.txt"

somehow it always takes me days to find string errors. The symptoms always seem to indicate something else.

Share this post


Link to post
Share on other sites
Quote:
Original post by Glak
ifstream config_file("config.tx"); //oops that should be "config.txt"

somehow it always takes me days to find string errors. The symptoms always seem to indicate something else.


This is one good reason to keep string literals out of your code as much as possible, and read your text data in from a file instead. (i18n concerns would be the other main reason.) You could also accept the "main config file name" from the command line ;)

Share this post


Link to post
Share on other sites
I hit a nasty one the other day:

void func1( const std::string &szFoo )
{
func2( szFoo.c_str() );
}

void main()
{
func1( "this is a string" );
}

This causes memory corruption, at least using the STL implementation in VC2005. Something about c_str() being marked const but actually doing something to the memory, which doesn't behave well with static strings.

Geoff

Share this post


Link to post
Share on other sites
Quote:
Original post by gdunbar
I hit a nasty one the other day:

void func1( const std::string &szFoo )
{
func2( szFoo.c_str() );
}

void main()
{
func1( "this is a string" );
}

This causes memory corruption, at least using the STL implementation in VC2005. Something about c_str() being marked const but actually doing something to the memory, which doesn't behave well with static strings.

Geoff

Is func2 a third-party function? It seems to use const_cast or do low-level manipulations (possibly in assembly) and I would stay away from code which did that.

As others have said there are just too many and you'll encounter them all the time. The most important thing is to learn from your mistakes. If I find myself often getting errors in my hard-coded strings then I try to avoid hard-coded strings and remember to pay extra attention every time I have to hardcode a string. If I had problems with template classes inside template classes then I would try to use typedefs and when I don't then pay extra attention.

Share this post


Link to post
Share on other sites
The Most Vexing Parse of C++ bit me yesterday, albeit in a slightly modified form.

Inside a member function, I had the following line:

scissor(bounds());

where scissor is a type and bounds is another member function of the same class. It seems like it should create a temporary scissor object, passing the result of bounds() as a parameter, but instead it is equivalent to this:

scissor bounds();

That is, a declaration of a function called bounds taking no parameters and returning a scissor object!

Quote:

std::vector<std::vector<int>> on some compilers won't parse correctly :(

It shouldn't, according to the Standard. Fortunately, the next revision will fix this. Meanwhile, as has been said, just put a space between the brackets.

Share this post


Link to post
Share on other sites
Quote:
Original post by Anonymous Poster
Add a space between the brackets and it'll work.

Yah, I was just pointing it out, I thought we were making a list of odd things. And I found that out like 4 years ago.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sharlin
Quote:

std::vector<std::vector<int>> on some compilers won't parse correctly :(

It shouldn't, according to the Standard. Fortunately, the next revision will fix this. Meanwhile, as has been said, just put a space between the brackets.


I rarely use nested templates. If I have to, I do something like this.

typedef std::vector<int> ivec;
std::vector<ivec> var;

If nothing else, it makes things more clear. In the same manner, I never rely on advanced precedence rules. If it's not obvious from looking at it, I use parentheses. It makes your code a lot easier to read and eliminates "what-you-say-isn't-what-you-meant" bugs.

Share this post


Link to post
Share on other sites
I think I got a nice one too:

class SomeClass
{
SomeClass(Device* pSomeUsefulDevice);
Device* pSomeUsefullDevice;
};

SomeClass::SomeClass(Device* pSomeUsefulDevice)
{
pSomeUsefulDevice = pSomeUsefulDevice; // own3d by copy-n-paste
};


I thought I had used the same identifier for my class member and my formal parameter. So the lazy guy I am just copy and pasted the identifier from the formal parameter... It took me a long time to figure out why my member variable was set back to an invalid state when the constructor exited...

Since that day, I use initialiser list when possible. I also use a "m" prefix to member variables and a "_" prefix to function arguments.

You also have the classic:
if( bValue = true) ...

Share this post


Link to post
Share on other sites
Being forced to write typename containerType::iterator in templates...

That one had me scratching my head for a long time, I thought my compiler had broken or something trying to figure out the error messages...

Share this post


Link to post
Share on other sites
Indeed, it's important to note that pretty much all of the listed gotchas (except the ever-present string literal typo) are c/c++ exclusive. Though to be fair other languages feature their own often less frequent gotchas (boxing in .NET, most builtin's of perl...)

Share this post


Link to post
Share on other sites
Quote:
Original post by janta
I think I got a nice one too:

class SomeClass
{
SomeClass(Device* pSomeUsefulDevice);
Device* pSomeUsefullDevice;
};

SomeClass::SomeClass(Device* pSomeUsefulDevice)
{
pSomeUsefulDevice = pSomeUsefulDevice; // own3d by copy-n-paste
};


I thought I had used the same identifier for my class member and my formal parameter. So the lazy guy I am just copy and pasted the identifier from the formal parameter... It took me a long time to figure out why my member variable was set back to an invalid state when the constructor exited...

Since that day, I use initialiser list when possible.


Note that it wouldn't work with that assignment even if you *did* have the same identifier name - the parameter name shadows the member name on the LHS just the same as on the RHS. AFAIK, the only way in C++ to make 'x = x' "do something" is to overload an operator=. Of course, with the initializer list, it *does* work, because the things-being-initialized *have* to be members, while the parameter shadows the member in the initialization-expressions.

Initialization lists also can be faster (if your compiler is dumb), *perform real initialization* (so there is no point in time when your object is in an invalid state, except time when it can't be considered an object at all), are cleaner (related to the previous - you wouldn't write "int x; x = 2;", right? Or even worse, "int x = 0; x = 2;", which is what constructor-assignment amounts to in the case where members are of class type), and are sometimes *necessary* (if a member-of-class-type or a base class lacks a default constructor). So yeah, using them is good. :) I know *you* learned the lesson, but it bears repeating to the masses.

Quote:
I also use a "m" prefix to member variables and a "_" prefix to function arguments.


You have to be careful with '_' prefixes. I like to use a '_' *suffix* - sometimes. But I think making appropriate use of 'this' is better than any kind of prefix or suffix convention, really - that's what 'this' is *there for*. It's so you can *not have to worry* about indicating that something is a member, when there is no ambiguity, *and still be clear* when there would otherwise be.

Quote:
You also have the classic:
if( bValue = true) ...


This is one of the reasons you should always write just 'if (bValue)' (or heck, even just 'if (value)' - ewww, Hungarian notation). It's at least as expressive (and I argue *more so*, based on analogy to English), shorter, avoids an opportunity to introduce an error, and is consistent with how you would use other if-expressions (you'd never write 'if ((x < y) == true)', for example; and variables by themselves count equally as "expressions" in the C++ grammar).

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Note that it wouldn't work with that assignment even if you *did* have the same identifier name - the parameter name shadows the member name on the LHS just the same as on the RHS.

Yeah ? In fact when I did that mistake it was when I was at the university taking java courses. I think in java it is different isn't it ? (or I was just plain wrong...) Anyway, since that day, in Java, i do use "this"

Quote:
This is one of the reasons you should always write just 'if (bValue)..

Yup in fact that's what I do. My point is valid in situation when something does not evaluate only to true or false, like

if(x = y)
...

Share this post


Link to post
Share on other sites
Quote:

Yup in fact that's what I do. My point is valid in situation when something does not evaluate only to true or false, like

if(x = y)
...



To avoid the =/== issue (which happens more often than I care to admit - especially as some of the other languages I use on day to day basis have '=' as the equality operator) I've gotten into the habit of putting an r-value on the left hand side of the == operator, so that I'll get a compiler error if I accidentally try to use the assignment operator.

Share this post


Link to post
Share on other sites
std::vector<int> vec;
vec.push_back(3);
vec.push_back(4);
for (std::vector<int>::iterator i = vec.begin(); i != vec.end(); i++)
{ ... }
ofcourse thats why you should use the standard library algorithms like std::for_each and std::transform

Share this post


Link to post
Share on other sites
Quote:
Original post by Julian90
std::vector<int> vec;
vec.push_back(3);
vec.push_back(4);
for (std::vector<int>::iterator i = vec.begin(); i != vec.end(); i++)
{ ... }
ofcourse thats why you should use the standard library algorithms like std::for_each and std::transform


Care to point out what's wrong with your code snippet? You seem to indicate that there's a problem with it, but I don't see it. It works just fine on VS2005.

Share this post


Link to post
Share on other sites
Quote:
Care to point out what's wrong with your code snippet? You seem to indicate that there's a problem with it, but I don't see it. It works just fine on VS2005.


Which demonstrates just how bad it is :), you should use pre-increment when iterating over sc++l containers (and anywhere else where post-increment is not necesarry) as post-increment is generally far harder to implement efficiently for udt's.

My other favorite (courtesy of washu)
int* array = new int[10]; int* p = array + 11;
whats the result of that and where does p point to?

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this