Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ