On Mon, 2014-11-03 at 13:32 -0700, Jeff Law wrote: > 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?
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). > > + > > +/* 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. (nods) > 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. Presumably by marking it with __attribute__((cold)) ? (with a suitable macro in case we're not being compiled with a gcc that supports it). I suspect doing so will be of more use in teaching me about gcc internals and the effect on generated code that in measurable benefits to said code in this case :) > > +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. Sorry. I'll post a followup with comments added. 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: /* Public entrypoint. See description in libgccjit.h. */ for each of these? (perhaps with additional text giving implementation notes?) 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). Dave