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
