[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130516131958.GB4978@openwall.com>
Date: Thu, 16 May 2013 17:19:58 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: coding style (was: password generation on GPU)
On Thu, May 16, 2013 at 06:11:48PM +0530, Sayantan Datta wrote:
> On Thu, May 16, 2013 at 4:38 PM, Solar Designer <solar@...nwall.com> wrote:
>
> > Alternatively, if you have free time now, how about you put some of it
> > into improving the quality of your code already in bleeding? Try to
> > correct things such as weird/inconsistent indentation, having too many
> > global symbols, weird naming, etc. I'm not sure if you notice those or
> > not, though.
>
> I'll try to solve this. BTW is there any tutorial regarding indentation etc
> which I should try to follow.
We use roughly the following indentation style in C sources:
indent -kr -i8 -nlp -nbbo -ncs -l79 -lc79
We should be able to do the same for OpenCL.
One of the major exceptions is the "struct fmt_main" initializers, which
have to be formatted manually (the "indent" command above turns them
into something awful). You may use e.g. dummy.c as an example of that.
To comment on some code you (Sayantan) edited, for example
opencl_bf_std.c has these weird things:
Unusual indentation in "typedef ... gpu_buffer;".
Extra indentation (equivalent to 2 or 3 tabs instead of just 1) in P_box
and S_box initializers.
Empty lines that don't improve code readability - e.g. 3 empty lines
after the S_box initializer (1 empty line would do just as well).
Extra indentation (1 tab instead of none) for the many "static cl_*"
declarations. Empty lines between those declarations - I think these
empty lines don't carry a meaning. Normally, empty lines should be used
to separate distinct things, and lack of an empty line may then be used
to group together similar/related things. When you instead separate
every declaration with an empty line, you merely increase the source
file length without making it any more readable, and you lose the
ability to group similar/related things together.
Instead of the many separate arrays of size MAX_PLATFORMS, shouldn't you
use one array of structs? Similarly, instead of having several arrays
of size MAX_DEVICES_PER_PLATFORM inside those structs, shouldn't you
have another array of structs in them? I think this would be cleaner.
In many places, there's no space character after a comma. This is
inconsistent with how the rest of the source code in JtR is formatted
(including the portions of code that you've taken for editing). For
example:
#define BF_ENCRYPT(ctx_S,ctx_P, L, R) \
L ^= ctx_P[pos_P(0)]; \
BF_ROUND(ctx_S,ctx_P, L, R, 0, u1, u2, u3, u4); \
BF_ROUND(ctx_S,ctx_P, R, L, 1, u1, u2, u3, u4); \
Why is there no comma after ctx_S, but there is after ctx_P? No good
reason, I think, so just add the comma. This is seen in a lot of other
places as well.
Similarly, we normally include one space character before/after each
operator in expressions, but many of your edited expressions lack them.
clean_gpu_buffer() repeats the message "Release Memory Object FAILED."
six times. (It also lacks space chars after commas in those calls.)
You could use something like:
const char *msg = "Release Memory Object FAILED.";
and then use "msg". ... or perhaps there's (should be) some interface
in common-opencl.c that we should use in such places?
get_dev_info() overlaps with code found in common-opencl.c, such as
get_processor_family(). (It also has the inner if/else not properly
indented in both files.) Perhaps you should revise opencl_bf_std.c to
use the common code more.
I think this is enough examples for now, so I'll stop here.
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.
