Yes, there was a bug in my vint64 encapsulation commit. I will neither confirm nor deny any conjecture that I left it in there deliberately to see who would be sharp enough to spot it. I will however note that it is a perfect tutorial example for how you should spot bugs, and why revisions with a simple and provable relationship to their ancestors are best
The following call in libntp/ntp_calendar.c is incorrect:
Now consider the line this replaced:
res.Q_s -= 0x80000000;
And notice what that expands to, semantically, in C:
res.Q_s = res.Q_s – 0x80000000;
Spotted it yet?
My encapsulation patch is extremely regular in form. One of my blog commenters (the only one to spot the bug, so far) pointed out correctly that an ideal transformation of this kind looks like it was done using a text editor search and replace feature – and, in fact, I did most of it with regexp-replace commands in Emacs.
It’s good when your patches are this regular, because it means that you can spot bugs by looking for irregularities – places where a local change breaks a rule followed in the rest of the patch. Importantly, this way to spot defects works even when you don’t fully understand the code.
This is a major reason the code state after every change should have a single provable relationship to its antecedent – because if it has more than one change in it, telltale irregularities will be harder to see.
OK, here is the corrected form of the call:
The one-character difference is that the correct inner call is to vint64u(), not vint64s(). You should have been able to spot this in one of a couple of ways.
One is by noticing that the original expression was doing unsigned arithmetic, so what is that call to get a signed value doing in there?
The even simpler way to spot the irregularity is to have noticed that in the rest of the diff there are no other calls like
setvint64X(res, vint64Y(res) … );
in which X and Y are unequal. There is a purely textual symmetry in the patch that this one statement breaks. Because the author was being careful about simplicity and provable relationships, that in itself should be enough to focus a reviewer’s suspicions even if the reviewer doesn’t know (or has forgotten) the meaning of the s and u suffixes.
I’m writing this as though the author and reviewer are different people, but these techniques for bug spotting – and, more importantly, these techniques for writing patches so bugs are easy to spot – apply even when you are your own reviewer and you are looking at a diff mere moments after you changed the code.
You get fast at coding, and you get good at doing it with a low defect rate, by developing the habits of mind that make self-checking like this easy. The faster you can self-check, the faster you can write while holding expected defect rates constant. The better you can self-check, the lower you can push your defect rate.
“To do large code changes correctly, factor them into a series of smaller steps such that each revision has a well-defined and provable relationship to the last” is good advice exactly because the “well-defined and provable” relationship creates regularities – invariants – that make buggy changes relatively easy to spot before you commit them.
I often go entire months per project without committing a bug to the repository. There have been good stretches on NTPsec in which my error rate was down around one introduced bug per quarter while I was coding at apparently breakneck speed. This is how I do that.
Having good tests – and the habit of adding a unit or regression test on every new feature or bug – helps a lot with that, of course. But prior to testing is good habits of mind. The combination of good habits of mind with good testing is not just additively effective, it’s multiplicatively so.