[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140921203959.GA10571@openwall.com>
Date: Mon, 22 Sep 2014 00:39:59 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Restart work on mask mode
Sayantan,
On Mon, Sep 22, 2014 at 12:22:22AM +0400, Solar Designer wrote:
> Just one detail I am actually able to communicate to you easily: please
> don't use nested functions.
Oh, here's some more:
Please don't forget to add yourself as a copyright holder for this
source file.
This (commented out) line is weird:
//#include <string.h> /* for isalpha(), isxdigit() */
Those macros actually come from <ctype.h>, not <string.h>.
These lines should have spaces added:
#include<string.h>
#include<stdlib.h>
The indentation levels are excessive - ideally, you'd re-arrange the
code such that you would not need to nest blocks this deeply.
Closely related to the above: many lines are overly long. In the rest
of JtR, I tried to keep lines in under 80 chars, and that's with 8 char
tabs. Sure, this is often difficult to do, but it provides a reminder
to restructure the code (usually to split out more portions of it into
functions) before it becomes too complex.
Some curly braces are mis-indented, most commonly before "else", e.g.:
end_c[k] = strtol(str, NULL, 16);
}
else {
dash_loc_beg[k] = j - 1;
would be corrected to:
end_c[k] = strtol(str, NULL, 16);
} else {
dash_loc_beg[k] = j - 1;
You pass potentially negative numbers into ctype macros:
static void init_cpu_mask(char *mask, [...]
[...]
isxdigit((int)mask[j - 2]) [...]
This causes sign extension. You need to cast to (int)(unsigned char)
(yes, two casts in a row), or avoid ctype macros there (but don't make
the same error with your own array lookups!)
In many places (but not in all), you have no whitespace between "if" or
"for" and the following opening brace. Elsewhere in JtR (and in the
rest of instances in this same source file), we use "if (" and "for (",
etc. with the whitespace.
The commented out portions of code need to be fully dropped, or at least
turned into #if 0 ... #endif blocks. They are not comments.
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.
