Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 5 Jun 2015 10:00:32 +0300
From: Solar Designer <>
Subject: Re: Coding Style

On Fri, Jun 05, 2015 at 02:28:23PM +0800, Kai Zhao wrote:
> magnum has run indent and astyle with two files.

I dislike that this preserves/uses alignment to opening brace - more on
this below.

> First:
>   indent -kr -i4 -ts4 -nlp -nbbo -ncs -l79 -lc79 -bad -il0
> common-opencl.[ch]
> and then:
>   astyle --style=kr -t4 -U -H -xC79 -c -k3 -z2 common-opencl.[ch]

What is it exactly that running astyle on top of indent buys us?

> 2. There are strings like: " supported with --platform " "syntax.\n"
> -------------------------------------------------------------------------------------
> original
> ----------
> void f2()
> {
>         fprintf(stderr, "Only one OpenCL device"
>                         " supported with --platform "
>                         "syntax.\n");
> }

I am not using alignment in core, and I don't intend to.

> formatted
> -------------
> void f2()
> {
>         fprintf(stderr, "Only one OpenCL device"
>                 " supported with --platform " "syntax.\n");
> }

It's OK that the alignment has changed, but we do indeed need to merge
the strings manually after that.

I'd prefer the alignment to be completely gone, though.  My old style
was to use a tab for continuation lines as well.  My newer style is to
use 4 spaces there, which is visibly different when tabs are 8 chars.
So this would be:

	fprintf(stderr, "Only one OpenCL device"
	    " supported with --platform syntax.\n");

Incidentally, 4 chars happens to match alignment to opening brace for

> Generally, I think the indent and astyle help a lot, and it only causes
> a few breakages. And we can use to check again.

What about the (questionable) construct like? -

		pw = salt->list;
		do {
			if (crk_methods.cmp_all(pw->binary, match))
			for (index = 0; index < match; index++)
			if (crk_methods.cmp_one(pw->binary, index))
			if (crk_methods.cmp_exact(crk_methods.source(
			    pw->source, pw->binary), index)) {
				if (crk_process_guess(salt, pw, index))
					return 1;
		} while ((pw = pw->next));

For the two sequential if's, we can just merge them into one with &&.
However, for if/for/if we'll get two extra levels of indentation here.
I suppose we sort of have to bite that bullet.  Well, maybe we could
save an indentation level (and, more importantly, make the code cleaner?
or not?) by moving the entire loop above into a separate function.


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.