On Tue, Jul 12, 2011 at 09:25, Michael Vacek wrote:
get the feedback below sorted out and i think i can live with the rest
(for now) to merge it
> This API, however, does not provide a suitable function needed. It was
> renamed and moved in jamutils.c.
hmm, thanks for pointing out this file. much of the API it provides
is covered by existing C library code.
#include <ctype.h>
jam_toupper -> toupper
jam_iscntrl -> iscntrl
jam_isalpha -> isalpha
jam_isdigit -> isdigit
jam_isalnum -> isalnum
jam_isspace -> isspace
jam_is_hex_char -> isxdigit
jam_is_name_char should be a static inline in jamutil.h which simply does:
isalnum(c) || c == '_'
#include <string.h>
jam_strlen -> strlen
jam_strcmp -> strcmp
jam_stricmp -> strcasecmp
jam_strncmp -> strncmp
jam_strnicmp -> strncasecmp
jam_strcpy -> strcpy
jam_strncpy -> strncpy
#include <stdlib.h>
jam_atol -> atol
jam_todigit can be a static inline in jamutil.h which simply does:
char c;
snprintf (&c, 1, "%u", value % 16);
return c;
jam_ltoa can be a static inline in jamutil.h which simply does:
sprintf (buffer, "%i", number);
> src/cmd/cmd_stapl.c
please dont copy the README blurb to all these files. in the leading
comment block, use a standard like you fixed include/urjtag/stapl.h to
use
> urj_chain_t * chain
no space after the "*" here. this shows up in a few different places.
"void *foo)" not "void * foo".
> src/stapl/jamarray.c
> jam_bool_rep_table[]
> src/stapl/jamexec.c
> jam_instruction_table[]
> src/stapl/jamexp.c
> jam_keyword_table[]
> src/stapl/jamjtag.c
> jam_jtag_state_table[]
> src/stapl/stapl.c
> error_text[]
these arrays are still being exported. looks like they should be
marked "static const".
> {
> JAM_ACTION_INSTR, "ACTION"},
all the arrays have this style. just use one line:
{ JAM_ACTION_INSTR, "ACTION" },
and make sure the trailing "};" is on a line by itself.
so something like:
static const int foo[] = {
0,
1,
2,
};
>#define JAMC_BOOL_REP_COUNT \
> ((int) (sizeof(jam_bool_rep_table) / sizeof(jam_bool_rep_table[0])))
>#define JAMC_INSTR_COUNT \
> ((int) (sizeof(jam_instruction_table) / sizeof(jam_instruction_table[0])))
>#define NUM_KEYWORDS ((int) \
> (sizeof(jam_keyword_table) / sizeof(jam_keyword_table[0])))
>#define JAMC_JTAG_STATE_COUNT \
> (sizeof(jam_jtag_state_table) / sizeof(jam_jtag_state_table[0]))
>#define MAX_ERROR_CODE (int)((sizeof(error_text)/sizeof(error_text[0]))+1)
>#define JAMC_BOOL_REP_COUNT \
> ((int) (sizeof(jam_bool_rep_table) / sizeof(jam_bool_rep_table[0])))
use ARRAY_SIZE(...) instead
> src/stapl/stapl.c
> jam_message (char *message_text)
> jam_export_integer (char *key, int32_t value)
mark that string const
> char *error_text[] = {
use "static const char * const error_text[] = {"
> jam_malloc (unsigned int size)
> jam_free (void *ptr)
no need for these. just use malloc()/free() directly.
> in general: "return (foo);" should be "return foo;"
seems like this is still valid. many can probably be fixed with a sed like:
sed -i '/return/s:(^[[:space:]]*return )[(](.*)[)];$:\1\2;:' *.c
> src/stapl/jamdefs.h
> extern char *jam_workspace;
all these exported types might not be easy to localize. so i'll
probably have to finish my hidden attribute work and we can convert
these to that.
i'm not sure this will work for static linking though. so please
namespace any exported symbols to "urj_jam_xxx". you can find out
what is being exported by running `readelf -sW .libs/liburjtag.so |
grep GLOBAL.*jam`.
> typedef int BOOL;
drop these and use stdbool.h and "bool". maybe this sed will help:
sed -i 's:\<BOOL\>:bool:g' *.[ch]
> src/stapl/jamsym.c
> extern BOOL jam_checking_uses_list;
please dont use externs in C files ... keep them in common local jam headers
> jam_free_symbol_table ()
need to use "(void)" and not just "()"
> src/stapl/jamexprt.h
> extern void Fail (int addr, short bufdat, short chipdat);
> extern void DisStr (int addr, char *str, int len);
> extern void DisScale (int current, int total);
looks like dead prototypes that should get punted
> src/stapl/jamexec.c
>jam_skip_instruction_name (char *statement_buffer)
>jam_find_keyword (char *buffer, char *keyword)
make those const
>jam_process_drscan_compare
this func has a bunch of places where it's got 4+ new lines in a row.
you only need one.
> src/stapl/jamexp.c
>
> +#define GET_FIRST_CH \
> + jam_token_buffer_index = 0; \
> + GET_NEXT_CH;
> +
> +#define GET_NEXT_CH \
> + CH = jam_parse_string[jam_strptr++]; \
> + jam_token_buffer [jam_token_buffer_index++] = CH; \
> + if (jam_token_buffer_index >= MAX_BUFFER_LENGTH) { \
> + --jam_token_buffer_index; \
> + --jam_strptr; \
> + } \
> + jam_token_buffer [jam_token_buffer_index] = '\0';
> +
> +#define UNGET_CH \
> + jam_strptr--; \
> + jam_token_buffer[--jam_token_buffer_index] = '\0';
> +
> +#define DELETE_CH jam_token_buffer [--jam_token_buffer_index] = '\0'
> +#define CH jam_ch
these need to be static inline funcs like so:
static inline char jam_get_first_ch(void)
{
jam_token_buffer_index = 0;
return jam_get_next_ch();
}
#define GET_FIRST_CH jam_get_first_ch()
....
> jam_exponentiate
> jam_square_root
> jam_exp_lexer
> + ACCEPT (END_OF_STRING) /* Fake an EOF! */
> + else
> + if (CH == ' ' || jam_iscntrl (CH))
that should be "else if" on one line
> + UNGET_CH;
> + ACCEPT ('&')}
this func has broken style in a bunch of places like this. indent the
ACCEPT and move the brace below it:
UNGET_CH;
ACCEPT ('&')
}
> if (jam_isalnum (CH) || CH == '_')
isnt this the same thing as jam_is_name_char() ?
> jam_constant_is_ok
> jam_binary_constant_is_ok
> jam_hex_constant_is_ok
these probably can be implemented with strspn(). so for
binary_constant_is_ok, do:
size_t ret = strspn (string, "10");
return string[ret] == '\0' ? true : false;
these helpers should all take a constant string and their return type
should be "bool", not "int"
> jam_atol_hex
use scanf("%x", &result) to basically do the same exact thing
> jam_yylex ()
> jam_yyparse ()
use "(void)" and not "()"
> jam_yyexca[]
> jam_yyact[]
> jam_yypact[]
> jam_yypgo[]
> jam_yyr1[]
> jam_yyr2[]
> jam_yychk[]
> jam_yydef[]
> +YYSTYPE jam_yyv[YYMAXDEPTH];
> +int token = -1; /* input token */
> +int errct = 0; /* error count */
> +int errfl = 0; /* error flag */
i think this should be static too ?
> +/* # line 288 "jamexp.y" */
i dont suppose you have jamexp.y instead ? we would want to keep that
in svn, not the generated jamexp.c ...
> src/stapl/jamheap.c
> +jam_init_heap (void)
> +jam_free_heap (void)
is there a reason you need this at all ? why cant it simply call
malloc() and free() like sane code ?
> src/stapl/jamjtag.c
> jam_jtag_state_transitions[]
> jam_jtag_path_map[]
mark static const
> src/stapl/jamsym.c
> jam_init_symbol_table ()
> jam_free_symbol_table ()
"()" -> "(void)"
> jam_hash (char *name)
mark "name" const
> src/stapl/stapl.c
> urj_stapl_run()
> + switch (exit_code)
> + {
> + case 0:
> + exit_string = "Success";
> + break;
> + case 1:
> + exit_string = "Checking chain failure";
> + break;
> + case 2:
create another array like error_text[] rather than encoding the lookup
table in a switch statement. then you can "exit_string =
exit_text[exit_code];".
-mike
------------------------------------------------------------------------------
AppSumo Presents a FREE Video for the SourceForge Community by Eric
Ries, the creator of the Lean Startup Methodology on "Lean Startup
Secrets Revealed." This video shows you how to validate your ideas,
optimize your ideas and identify your business strategy.
http://p.sf.net/sfu/appsumosfdev2dev
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
