On Reviewing a Patch in Qt

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, ...)
But all of this is the least important part of reviewing. Because the submitter is already supposed to know those policies that should be documented for any software project.

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.

Inside Nokia we used to have an internal Pastebin website for putting up patches. Nowadays the Qt Project is using a much more advanced process for reviewing.

Woboq is a software company that specializes in development and consulting around Qt and C++. Hire us!

If you like this blog and want to read similar articles, consider subscribing via our RSS feed (Via Google Feedburner, Privacy Policy), by e-mail (Via Google Feedburner, Privacy Policy) or follow us on twitter or add us on G+.

Submit on reddit Submit on reddit Tweet about it Share on Facebook Post on Google+

Article posted by Olivier Goffart on 23 January 2012

Load Comments...
Loading comments embeds an external widget from disqus.com.
Check disqus privacy policy for more information.
Get notified when we post a new interesting article!

Click to subscribe via RSS or e-mail on Google Feedburner. (external service).

Click for the privacy policy of Google Feedburner.
© 2018 Woboq GmbH Google Analytics Tracking Opt-Out