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:
setvint64u(res, vint64s(res)-0x80000000);
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
<em>single</em> 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:
setvint64u(res, vint64u(res)-0x80000000);
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.
--
<a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>
It is error alone which needs the support of government.
Truth can stand by itself.
--Thomas Jefferson
_______________________________________________
devel mailing list
devel@ntpsec.org
http://lists.ntpsec.org/mailman/listinfo/devel
