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.