On Reviewing a Patch in Qt
Posted by Olivier Goffart on 23 January 2012
While I was working on Qt at Nokia I spent a lot of time reviewing patches from colleagues.
Peer reviewing is an important step, and there is a reason why we do it in Qt.
Code in libraries (such as Qt) requires much greater care than application code.
The main reason is that you need to maintain compatibility for your users. You want to avoid regressions in your library, so code that works continues to work, even if the user did some hack or trick. For the user of the library, it has great value to be able to upgrade it without too much effort. A subtle behaviour difference in the library might introduce a bug in the application that might be really hard to track in the millions of old lines of code of the application.
Reviewing changes starts by looking for obvious mistakes and incompatibilities with the project guidelines:
- Is the code style correct?
- Is the code (auto)tested?
- Are the new APIs properly documented?
- Does the code follow the project policy? (Thread safety, no static global objects, no exported symbols without prefix, No use of features not supported by a supported compiler, ...)
The hard part of the review is seeing the things that the author of the patch could not possibly know.
It takes someone already familiar with the code to do the review. Someone who knows how the code interacts with the other parts, someone who knows the rationale behind some of the part of the code.
Even if you are familiar with the code, you almost always need to open the IDE and browse the existing code and the code it interacts too. Making a review by just looking at the patch may be possible for trivial compilation fix, but in almost all case of non trivial code, you will need to open the relevant files on your IDE.
There is so many things to check and to be aware of: code calling virtual functions, code used
for other use cases than the one of the bug report, ...
You also need to think hard about what the patch might break and what the behaviour changes are.
As an example, let us take someone who does a change in a QComboBox to fix a usability glitch on Windows. Someone not familiar with QComboBox might not know that it behaves very differently depending on the style. The reviewer will likely to be able to spot that a change will "break" that style.
Another example is someone who identifies that a bug is caused by one wrong condition in an if clause. Changing seems to fix the problem. But why was that condition there? If the code is not commented and the history of the code shows it is very old, no relevant information can be deduced. The reviewer should be the one who knows that this condition is there to test a very specific use case that might or might not be required anymore.