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

[Jeff Law]

On 11/04/14 09:57, David Malcolm wrote:
>>> +#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?
>
> I've been burned in the past by the C library using locales, in
> particular the two lowercase "i" variants in Turkish.
>
> These macros are used by gcc_jit_context_new_function to enforce C's
> naming restrictions, to avoid errors from the assembler.  The comment I
> put there was:
>
>    /* The assembler can only handle certain names, so for now, enforce
>       C's rules for identifiers upon the name.
>       Eventually we'll need some way to interact with e.g. C++ name mangling.  */
>
> Am I right in thinking that for the assembler we need to enforce the C
> naming rules specifically on *ASCII*.
>
> (clearly another comment is needed here).
I guess you've got to do it somewhere.  Presumably there isn't something 
already in GCC that enforces an input character set?  I guess I just 
dislike seeing something that feels like it ought to already be available.

>
> Presumably by marking it with __attribute__((cold)) ?  (with a suitable
> macro in case we're not being compiled with a gcc that supports it).
Yup.  That's precisely what you want since that gives the predictors 
enough information to mark paths as unlikely without having to mark each 
path yourself.

>
> Sorry.  I'll post a followup with comments added.
Thanks.  I probably rely more on those for this kind of review than 
anything, so the lack of them really stood out.

>
> Many of the functions are public API entrypoints, where there's a
> comment in the public header.  Should I simply duplicate the comment
> from there into the .c file, or put a comment like:
Good question.  Normally in the past we'd have you duplicate the 
comment, but with this new usage scenario that may not make a lot of 
sense since one or the other will likely get out of sync at some point.

At this point a snarky comment about generating documentation and the 
interface from a single definition would be appropriate.


>
>    /* Public entrypoint.  See description in libgccjit.h.  */
>
> for each of these?  (perhaps with additional text giving implementation
> notes?)
Let's go with this.  If folks want the comment duplicated, they can 
argue for it after the fact :-)

>
>
> Thanks for all the reviews.  Looks like this and patch 16 are now the
> only non-approved parts of the kit (I didn't see a review of 16).
Right.  I didn't get to #16 yesterday.

jeff