Why coding style matter?
This is a lesson I’ve learned recently and is somewhat related with another thing that happened to me in the past. So, several months ago I was working on a piece of code and suddenly the code crashed in a function call in EASTL. Not giving any thought to the problem I said to myself that the problem must be in EASTL. Even if I had the sources I wasn’t familiar with the details so I just send an error report to the original author.
The author just told me that if I can replicate that problem in an unit test case then he would take a look at it. So I was stuck because the problem was in the implementation of a hash_map and I was using several instances of hash_map in my application but only this one crashed. This was a hash_map holding textures so I was assuming that maybe something was wrong with the implementation of the texture class. To make the matters worse when a colleague of mine moved some lines up and down or commented them the problem suddenly vanished. Because we were pressured by time we let this one go, thinking that if it has something to do with the implementation of the hash_map somebody else will bump into it and we’ll try to solve it then.
Then after a couple of months another colleague found something unusual in the implementation of the same texture module, a class for loading a picture format. Some struct was guarded by a #pragma pack(push, 1) but there was no #pragma pack(pop) at the end of the struct. As you probably know this directive is telling the compiler to let the struct unaligned (the default behavior is to align the struct at say 4 bytes – platform dependent – because of the faster access). This is useful if you want to read a struct from a file in one go just mapping the bytes on the structure. Is especially useful when reading headers of files and things like that so it is no surprise that it was used for texture loading.
Read more about pragmas at http://msdn.microsoft.com/en-us/library/d9x1s805.aspx
Anyway we figured that this was the original cause for the hash_map crash. Probably the implementation of the hash_map did some assumptions about the alignment of structures.
The thing that stuck with me from all this situation is that at the moment I was trying to figure out a way to prevent this. What do I or anyone else could have done to prevent these kind of problems or to, at least, minimize the effects?
Last week I had a discussion at work about the usefulness of forward declaring classes in C++ and in the discussion we kept enumerating good reasons for doing this. Everybody will tell you about compilation speed but I jumped ahead and said something like “well it’s good to do this because you only include what you really need and in this way you can prevent all sorts of bad things”. And everybody kept waiting looking at me: “like what?”. And I start thinking at things and then the initial story pops into my head. If we have kept the includes at a minimum that problem couldn’t have manifested itself because probably the problem was that the header file was included in other header files and the pragma option applied to several other structures that we were using or even structures in EASTL.
So this was my answer. We could have kept the effects at a minimum if we respected a simple coding style rule: Forward declare everything you can!
BTW we’ve started doing this after we’ve committed ourselves to use Google C++ Coding Style.
The other solution to these kinds of problems is to have a static analyzer checking your code periodically but you don’t know if the static analyzer will check for these kinds of issues, so respecting strong coding guidelines is better.