[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <51C744DB.6050106@gmail.com>
Date: Mon, 24 Jun 2013 00:26:27 +0530
From: Sayantan Datta <std2048@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: coding style
On Thursday 16 May 2013 06:49 PM, Solar Designer wrote:
> 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.
This command breaks a long line of codes into multiple shorter lines but
at wrong positions making the code even more unreadable. So I didn't
apply this command.
>
> 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.
Okay. It was already fixed.
>
> To comment on some code you (Sayantan) edited, for example
> opencl_bf_std.c has these weird things:
>
> Unusual indentation in "typedef ... gpu_buffer;".
Fixed.
>
> Extra indentation (equivalent to 2 or 3 tabs instead of just 1) in P_box
> and S_box initializers.
Fixed.
>
> 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.
Fixed in all three of my formats.
>
> 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.
Most of the global variables were redundant. So I removed most of them.
>
> 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.
I fixed these issues at most of the places.
>
> Similarly, we normally include one space character before/after each
> operator in expressions, but many of your edited expressions lack them.
Fixed.
>
> 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
Removed all redundant opencl functions.
From now on I try to keep those things in mind while writing code.
Regards,
Sayantan
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.
