On Reviewing a Patch in Qt

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.