Re: [PATCH 10/27] New file: gcc/jit/libgccjit.c

[Jeff Law <law at redhat dot com>]

On 10/31/14 11:02, David Malcolm wrote:
> This file implements the entrypoints of the library's public API.
>
> It performs error-checking at this boundary, before calling into the
> jit-recording.h internal API.
>
> gcc/jit/
> 	* libgccjit.c: New.
> ---
>   gcc/jit/libgccjit.c | 1506 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 1506 insertions(+)
>   create mode 100644 gcc/jit/libgccjit.c
>
> +
> +#define IS_ASCII_ALPHA(CHAR) \
> +  (					\
> +    ((CHAR) >= 'a' && (CHAR) <='z')	\
> +    ||					\
> +    ((CHAR) >= 'A' && (CHAR) <= 'Z')	\
> +  )
> +
> +#define IS_ASCII_DIGIT(CHAR) \
> +  ((CHAR) >= '0' && (CHAR) <='9')
> +
> +#define IS_ASCII_ALNUM(CHAR) \
> +  (IS_ASCII_ALPHA (CHAR) || IS_ASCII_DIGIT (CHAR))
Can't we rely on the C library to give us equivalents?

> +
> +/* TODO: mark failure branches as unlikely? */
> +
Not likely worth the effort.  And it'd be better to somehow mark 
jit_error such that anytime a path unconditionally calls jit_error, the 
whole path is considered unlikely.

I think it was Ball & Larus that had a predictor of this nature, I don't 
recall its effectiveness offhand.  But I'd rather be marking the 
function than sprinlkling unlikely all over the actual codepaths.



> +static bool
> +compatible_types (gcc::jit::recording::type *ltype,
> +		  gcc::jit::recording::type *rtype)
All function definitions should have a block comment describing the 
function and its arguments.   This comment applies throughout the code 
and needs to be addressed prior to commit.  In fact I really can't even 
continue review of this code due to the lack of comments -- I rely 
heavily on them.

jeff