[MaxBarraclough]

Disagree. A code-review is not purely checking for non-functional properties of the code, it's checking for overall code quality, and that includes bugs.

Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug.

You really want to deliberately discard this bug-finding opportunity? Why?

Even if it's a false positive, doesn't that indicate that something bears commenting?

[taneq]

> Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug.

And most often of all, while explaining what they've done, the reviewee will say "and this does... oh crap, wait" and fix the mistake themselves. Among all their other virtues, a code reviewer is a level 2 debugging duck. (Although all of my code review experience is with buddy check-in style reviewing rather than sending it off to be reviewed. I can understand endless nitpicky questioning being annoying if you're doing it offline.)

[benji-york]

This "oh crap" effect is one of the reasons I like "self review" as the first step in a code review process.

That is, go over the diff and add commentary explaining to the reviewer why you made particular changes (not explaining what the changes do, that should be in comments in the source).

[MaxBarraclough]

GitHub supports posting comments to a commit. Might be a good fit.

[ams6110]

> Occasionally a reviewer will spot a bug

Isn't the code review generally pretty late, i.e. just prior to release? At that point, the code should be passing all unit tests and I'd expect obvious bugs to be pretty unlikely. Non-obvious bugs generally won't be spotted in a code review setting.

[MaxBarraclough]

Not impossible that the reviewer might spot a bug. There's value in more eyeballs. Useful for spotting dangerous constructs or things like UB in C/C++ that appear to work fine... for now.

Also, lots of software houses don't have formal testing of all new code. Not unusual with GUI code, for instance.

[spathi_fwiffo]

Not sure what you mean by 'late'. I guess it all depends on process defined for your project. Most SCM systems have some way to make code visible before it is published to the live code base (via branches, etc).

Also, passing tests is meaningless if the tests are worthless or incomplete. So, those alone can't be used as a metric of code sanity; Meaning obvious bugs are still possible.

Tests and reviews are just to reduce the number of released bugs. and hopefully increase maintainability.

Also, what's obvious to one developer might not be to another.