Last active
August 29, 2015 14:04
-
-
Save nothings/28d163780c35a8608271 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
One of the primary benefits of using a fixed-width | |
programming font is that things line up in columns | |
if they are the same length in characters. (Please | |
note that monospacing is not just for indentation; | |
indentation works fine with proportional fonts. It | |
is only once you have a non-blank character on the | |
line that proportional vs. monospace differ much.) | |
In my experience, lining code up so features align | |
vertically makes it a lot easier to avoid the bugs | |
described here: http://www.viva64.com/en/b/0260/ . | |
I've lined up all the attributed examples shown on | |
that page. There are two considerations. First, is | |
the bug immediately obvious? Second, does scanning | |
vertically down the "changing" columns turn up the | |
bug with little mental effort (i.e. mechanically)? | |
Of the 15 attributed examples, my reading is that: | |
3 are not helped by column-scanning; | |
1 becomes very obvious once aligned | |
6 are obvious when column-scanned (but 4 were already aligned) | |
5 are obvious when column-scanned, but require multiple-statements per line or very long lines | |
Given that there were 4 examples that were already | |
aligned where the bug was easily found by scanning | |
columns, it's clear that aligning vertically isn't | |
a panacea: programmers must still scan the columns | |
to find the bug. So I firmly recommend they do so! | |
Note that this reasoning is one of the two primary | |
reasons that you can find multiple statements on a | |
single line in the stb_ libraries. (De-emphasizing | |
error-control-flow is the other important reason.) | |
Here is the sample code reformatted for alignment: | |
== Examples that are obvious once aligned. | |
// MongoDb | |
return _id == r._id | |
&& votes == r.votes | |
&& h == r.h | |
&& priority == r.priority | |
&& arbiterOnly == r.arbiterOnly | |
&& slaveDelay == r.slaveDelay | |
&& hidden == r.hidden | |
&& buildIndexes == buildIndexes; | |
== Obvious on column-scanning | |
// SlimDX | |
//| | | |
sliders[i] = joystate.rglSlider [i]; // if aligning is a priority, rename this to be same length | |
asliders[i] = joystate.rglASlider[i]; | |
vsliders[i] = joystate.rglVSlider[i]; | |
fsliders[i] = joystate.rglVSlider[i]; | |
// Qt | |
// | | | |
qreal x = ctx->callData->args[0].toNumber(); | |
qreal y = ctx->callData->args[1].toNumber(); | |
qreal w = ctx->callData->args[2].toNumber(); | |
qreal h = ctx->callData->args[3].toNumber(); | |
if (!qIsFinite(x) || | |
!qIsFinite(y) || | |
!qIsFinite(w) || | |
!qIsFinite(w) ) | |
// | | |
== Obvious, but only if you break the one-statement-per-line rule | |
// Chromium | |
if (access & FILE_WRITE_ATTRIBUTES) output.append(ASCIIToUTF16("\tFILE_WRITE_ATTRIBUTES\n")); | |
if (access & FILE_WRITE_DATA) output.append(ASCIIToUTF16("\tFILE_WRITE_DATA\n" )); | |
if (access & FILE_WRITE_EA) output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n" )); | |
if (access & FILE_WRITE_EA) output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n" )); | |
break; | |
== Obvious on column-scanning, but only if you break the one-statement-per-line rule | |
// Qt | |
// | | | | |
if (repetition.isEmpty()) { pattern->patternRepeatX = true ; pattern->patternRepeatY = true ; } | |
else if (repetition == QStringLiteral("repeat") { pattern->patternRepeatX = true ; pattern->patternRepeatY = true ; } | |
else if (repetition == QStringLiteral("repeat-x")) { pattern->patternRepeatX = true ; } | |
else if (repetition == QStringLiteral("repeat-y")) { pattern->patternRepeatY = true ; } | |
else if (repetition == QStringLiteral("no-repeat")) { pattern->patternRepeatY = false; pattern->patternRepeatY = false; } | |
else { | |
//TODO: exception: SYNTAX_ERR | |
} | |
// OpenSSL | |
// | | | | |
if (!strncmp(vstart, "ASCII" , 5)) arg->format = ASN1_GEN_FORMAT_ASCII; | |
else if (!strncmp(vstart, "UTF8" , 4)) arg->format = ASN1_GEN_FORMAT_UTF8; | |
else if (!strncmp(vstart, "HEX" , 3)) arg->format = ASN1_GEN_FORMAT_HEX; | |
else if (!strncmp(vstart, "BITLIST", 3)) arg->format = ASN1_GEN_FORMAT_BITLIST; | |
// Requires some additional work beyond just naive column-analysis; | |
// nevertheless, columnizing encourages you to work down sequentially | |
// through every number and check it, in an organized way which is | |
// harder when it's multiline, so I'm going to count it. | |
== Obvious on column-scanning, but only if you allow long lines | |
// Source Engine SDK | |
// | | | | |
intens.x=OrSIMD(AndSIMD(BackgroundColor.x,no_hit_mask),AndNotSIMD(no_hit_mask,intens.x)); | |
intens.y=OrSIMD(AndSIMD(BackgroundColor.y,no_hit_mask),AndNotSIMD(no_hit_mask,intens.y)); | |
intens.z=OrSIMD(AndSIMD(BackgroundColor.y,no_hit_mask),AndNotSIMD(no_hit_mask,intens.z)); | |
// Clang | |
// | | | | | |
return ContainerBegLine <= ContaineeBegLine | |
&& ContainerEndLine >= ContaineeEndLine // | | | | | |
&& (ContainerBegLine != ContaineeBegLine || SM.getExpansionColumnNumber(ContainerRBeg) <= SM.getExpansionColumnNumber(ContaineeRBeg)) | |
&& (ContainerEndLine != ContaineeEndLine || SM.getExpansionColumnNumber(ContainerREnd) >= SM.getExpansionColumnNumber(ContainerREnd)); | |
// this requires such long lines that I would probably break these evals into | |
// temporary variables that I could line up while having shorter lines. | |
// Also: don't use both Container and Containee. It's like using 'l' as a var. | |
== Examples that were already aligned in columns, | |
== column-scanning would catch: | |
// Source Engine SDK | |
// inline void Init | |
// SeqAn | |
// inline typename Value<Pipe>::Type const & operator* | |
// Quake-III-Arena | |
// if (fabs(dir[0]) | |
// Unreal Engine 4 | |
// static bool PositionIsInside( | |
== Examples that were already aligned in columns, | |
== but column-scanning wouldn't catch: | |
// ReactOS | |
// *ScanString | |
// (noisy, unalignable lexemes -- a reason to make #defines for them?) | |
// Trans-Proteomic Pipeline | |
// void setPepMaxProb | |
// (intervening code between lines makes it impossible to make adjacent) | |
// Mozilla Firefox | |
// if (protocol | |
// (column scanning only catches single-char patterns and adjacent-line repeats; | |
// note that ordering the comparisons alphabetically would help with this) | |
== Omitted from analysis because it's | |
== not clear what the bug actually is. | |
// Multi Theft Auto | |
// class CWaterPolySAInterface | |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment