[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5526B904.1000804@openwall.com>
Date: Thu, 09 Apr 2015 20:38:12 +0300
From: Alexander Cherepanov <ch3root@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Coding Style
On 2015-04-05 20:45, Solar Designer wrote:
> On Sun, Apr 05, 2015 at 08:28:06PM +0300, Alexander Cherepanov wrote:
>> I don't think such issues could be discussed in isolation. One of the
>> peculiarities of your style is the absence of indent in the following
>> code (from http://www.openwall.com/lists/john-dev/2011/07/15/2 ):
>>
>> for (i = 0; i < 10; i++)
>> for (j = 0; j < 10; j++)
>> for (k = 0; k < 10; k++) {
>> ... body ...
>> ... body ...
>> }
>>
>> This compensates for wide tabs to some degree. But I find this approach
>> very strange. Surely, the example above looks nice. But your next example:
>>
>> for (i = 0; i < 10; i++)
>> if (ok(i))
>> while (j))
>> ... body ...
>> ... body ...
>> }
>>
>> doesn't look better without indentation.
>
> To me, it looks fine without indentation
Sure, not everybody agree that systematic indentation is necessary at
all. Or that goto is harmful. Take f.e. tex.web.
But it doesn't mean that such an approach is practical in non-solo
projects or that it scales well.
> (but you lost the opening curly brace there).
Yeah, I have not bothered to insert it after copy-pasting from the URL
cited above.
> I simply treat the for/if/while lines of it as one
> complicated loop/condition for the following block.
This can have counterintuitive consequences sometimes. E.g. break
somewhere inside such a construction will not exit from this one loop if
there are two for/while in it.
>> And combined with the fact that there are fragments like this:
>>
>> for (pos = &ciphertext[4]; atoi16[ARCH_INDEX(*pos)] != 0x7F; pos++);
>> if (*pos || pos - ciphertext != 20) return 0;
>
> These are actually no good. Today, I'd format them as:
>
> for (pos = &ciphertext[4]; atoi16[ARCH_INDEX(*pos)] != 0x7F; pos++)
> continue;
> if (*pos || pos - ciphertext != 20)
> return 0;
>
> or better yet, rewrite the "for" loop as "while":
>
> pos = ciphertext + 4;
> while (atoi16[ARCH_INDEX(*pos)] != 0x7F)
> pos++;
Nice to hear it.
> BTW, we need to replace ARCH_INDEX() with an arch-neutral macro, perhaps
> in common.h.
The first question here which type to use. What are the reasons not to
use unsigned char or uint8_t and why it's arch-specific? (I remember the
discussion in http://seclists.org/nmap-dev/2009/q3/209 but I don't find
arguments there very convincing.)
The next question is how to get this type. I think it would be nice to
get it automatically, e.g. by pos being of type uint8_t *. Or even by
converting valid() and similar functions to get parameters as uint8_t *.
>> it greatly undermines the usefulness of indentation -- you cannot be
>> sure anymore where a block ended without fully parsing all the code.
s/block/statement/
> With the above corrections, I think you can. So I think corrections
> like these is what we should make.
Yes, these corrections are nice to have regardless of indentation in
other places.
>> I think the principle that you can find the end of the block just by
>> mentally drawing a vertical line until you meet something with less or
>> equal indentation
>
> This is a required property, yes.
Your approach make this property considerably more complicated:
1) the part "until you meet something with less or equal indentation"
changed to "until you meet something with bigger indentation and then
with less or equal indentation";
2) you have to differentiate between types of statements: expression
statements cannot be stacked, loops/conditions can, e.g. in code like this:
do_something();
call_a_function(with, many,
many, arguments);
if (a)
while (b)
do();
3) it gets messy when there are several do-while loops:
for (...)
do {
for (...)
do {
....
} while (...);
} while (...);
More real example -- should ifs be moved to the left here:
do {
if ((c_ops[op].class != C_CLASS_LEFT && left) ||
(c_ops[op].class == C_CLASS_LEFT && !left))
if (!memcmp(c_ops[op].name, token, strlen(c_ops[op].name)))
if (best < 0 ||
strlen(c_ops[op].name) > strlen(c_ops[best].name))
best = op;
} while (c_ops[++op].prec);
Hm, another instance but without extra indentation and with interesting
braces:
do
if (*token != ' ') {
...
} while (!c_errno && *(token = c_gettoken()) != ';');
4) what if one of if's in a stack gets else:
for (...)
if (...)
while (...)
...
else
...
I guess the list could go on.
>> overweights the usefulness of alignment like the first example above.
>
> What "first example above"? The one with "for ... ;" all on one line?
> Yes, we should get rid of those.
No, the example with three aligned for's. It looks nice but IMHO doesn't
worth breaking indentation rules.
--
Alexander Cherepanov
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
