Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 09 Apr 2015 20:38:12 +0300
From: Alexander Cherepanov <>
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 ):
>>    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 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.


> 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:

   call_a_function(with, many,
     many, arguments);
   if (a)
   while (b)

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 

         if (*token != ' ') {
         } while (!c_errno && *(token = c_gettoken()) != ';');

4) what if one of if's in a stack gets else:

   for (...)
   if (...)
   while (...)

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.