[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150605070032.GA13182@openwall.com>
Date: Fri, 5 Jun 2015 10:00:32 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
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.
>
> https://github.com/magnumripper/JohnTheRipper/commit/feb8a1521f7e69642cc430970cc1714c14698466
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
if's.
> Generally, I think the indent and astyle help a lot, and it only causes
> a few breakages. And we can use checkpatch.pl 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;
else
break;
}
} 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.
Alexander
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.
