[C++] Additional parenthesis are always safe! Really?

Background

In general, a programmer would be advised to provide additional parenthesis in the code "when in doubts" with a presumption that it is safe. We will see on example here on how in certain scenarios it cannot be safe and programmer needs to be extra careful.

Consider this code (which does nothing but puts a map of class pointer index versus class pointer index)

Sample 1

1:  #include <iostream>  
2:  #include <unistd.h>  
3:  #include <map>  
4:  using namespace std;

.

.
51:  int main()  
52:  {  
53:       const unsigned int MAX_COUNT = 20000;  
54:       map <I*,J*> contextHolder;  
55:       char *p[MAX_COUNT];  
.

.

. 

62:       map <I*,J*>::iterator itr = contextHolder.begin();  
63:       map <I*,J*>::iterator itrEnd = contextHolder.end();  
64:       while(itr != itrEnd)  
65:       {  
66:            if(someCondition)  
67:            {  
68:                 itr->first->f();  
69:                 itr->second->f();  
70:                 I* pTempI = itr->first;  
71:                 J* pTempJ = itr->second;  
72:                 delete pTempI;  
73:                 delete pTempJ;  
74:                 contextHolder.erase(itr);  
75:            }  
76:            itr++;  
77:       } 

.

.

.
82:       return 0;  
83:  } 

Now, let us discuss the code which is highlighted.
contextHolder.erase(itr);

This would lead to well known problem - crash,

• Line 74 deletes the contents pointing to itr - thereby invalidating the pointer.
• Line 76 increments the invalidated pointer - thus causing the crash.

to solve this problem, developers are known to add the iteration increment during the erase operation itself. Like

Sample 2

64:       while(itr != itrEnd)  
65:       {  
66:            if(someCondition)  
67:            {  
68:                 itr->first->f();  
69:                 itr->second->f();  
70:                 I* pTempI = itr->first;  
71:                 J* pTempJ = itr->second;  
72:                 delete pTempI;  
73:                 delete pTempJ;  
74:                 contextHolder.erase(itr++);  
75:            }  
76:          else  
77:          {  
78:           //do something else.  
79:             itr++;  
80:          }  
81:       }  

The fix in line 74 (In sample 2 versus sample 1) solves the problem with an increment operator for the iterator (because increment is done before invalidating the iterator).  

Detailed Description

All is well till now. But assume that a amateur programmer, who is confused about the working of increment operation in a complex statement like that of Sample 2, line 74 adds an addition braces for the increment iterator.


Sample 3

64:       while(itr != itrEnd)  
65:       {  
66:            if(someCondition)  
67:            {  
68:                 itr->first->f();  
69:                 itr->second->f();  
70:                 I* pTempI = itr->first;  
71:                 J* pTempJ = itr->second;  
72:                 delete pTempI;  
73:                 delete pTempJ;  
74:          contextHolder.erase((itr++));  
75:            }  
76:          else  
77:          {  
78:           //do something else.  
79:             itr++;  
80:          }  
81:     

We see that the application crashes at line 74.

Analysis 

Now this seemingly innocent braces (highlighted in red) looks to do more harm than good. This is a classical coding problem - the iterator is extended (incremented) beyond the boundary (i.e itr.end() ) and then the map tries to erase the same element - "causing crash".

So adding this additional parenthesis lead to a disastrous bug. Moreover, hard to debug/difficult to reoccur.Any static check tool also will mostly will fail to unearth the problem.

Take Away's 

• These kind of problems are very very difficult to catch at any cycle of SDLC phase. Even a reviewer (with immense experience) also will find difficult to catch these kind of issues.

• It is best to share these kind of unique issues which occur in projects as technical sharing across the organisation - so that developers are sensitised (adj : having an allergy or peculiar or excessive susceptibility :)), to such potential problem in code.

• It is best for the developer not to blindly follow "any rule", but to understand the concept behind the logic and how compiler interprets the code - in this case - the priority of execution in the complex execution statement.

Regards,
Tech Unravel
Supreme Debugging