On Fri, 2020-10-02 at 16:17 -0400, David Malcolm wrote:
> On Tue, 2020-09-01 at 21:01 -0400, Antoni Boucher via Jit wrote:
> > Hello.
> > This WIP patch implements new reflection functions in the C API as
> > mentioned in bug 96889.
> > I'm looking forward for feedbacks on this patch.
> > It's WIP because I'll probably add a few more reflection functions.
> > Thanks.
>
> Sorry about the belated review, looks like I missed this one.
>
> At a high level, it seems reasonable.
>
> Do you have a copyright assignment in place for GCC contributions?
> See https://gcc.gnu.org/contribute.html
>
> [...]
diff --git a/gcc/jit/docs/topics/compatibility.rst
> > b/gcc/jit/docs/topics/compatibility.rst
> > index bb3387fa583..7e786194ded 100644
> > --- a/gcc/jit/docs/topics/compatibility.rst
> > +++ b/gcc/jit/docs/topics/compatibility.rst
> > @@ -219,3 +219,14 @@ entrypoints:
> > * :func:`gcc_jit_version_minor`
> >
> > * :func:`gcc_jit_version_patchlevel`
> > +
> > +.. _LIBGCCJIT_ABI_14:
> > +
> > +``LIBGCCJIT_ABI_14``
> > +--------------------
> > +``LIBGCCJIT_ABI_14`` covers the addition of reflection functions
> > via API
> > +entrypoints:
> > +
> > + * :func:`gcc_jit_function_get_return_type`
> > +
> > + * :func:`gcc_jit_function_get_param_count`
>
> This will now need bumping to 15; 14 covers the addition of
> gcc_jit_global_set_initializer.
>
> [...]
>
> > +/* Public entrypoint. See description in libgccjit.h.
> > +
> > + After error-checking, the real work is done by the
> > + gcc::jit::recording::function::get_return_type method, in
> > + jit-recording.h. */
> > +
> > +gcc_jit_type *
> > +gcc_jit_function_get_return_type (gcc_jit_function *func)
> > +{
>
> This one is missing a:
> RETURN_NULL_IF_FAIL (func, NULL, NULL, "NULL function");
>
>
> > + return (gcc_jit_type *)func->get_return_type ();
> > +}
>
> [...]
>
> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > index 1c5a12e9c01..6999ce25ca2 100644
> > --- a/gcc/jit/libgccjit.h
> > +++ b/gcc/jit/libgccjit.h
>
> [...]
>
> > @@ -1503,6 +1511,22 @@ gcc_jit_version_minor (void);
> > extern int
> > gcc_jit_version_patchlevel (void);
> >
> > +#define LIBGCCJIT_HAVE_gcc_jit_function_reflection
> > +
> > +/* Reflection functions to get the number of parameters and return
> > types of
> > + a function from the C API.
>
> "return type", is better grammar, I think, given that "function" is
> singular.
>
> > +
> > + "vec_type" should be a vector type, created using
> > gcc_jit_type_get_vector.
>
> This line about "vec_type" seems to be a leftover from a copy&paste.
>
>
> > + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test
> > for its
> > + presence using
> > + #ifdef LIBGCCJIT_HAVE_gcc_jit_function_reflection
>
> Version number will need bumping, as mentioned above.
>
> [...]
>
> > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > index 6137dd4b4b0..b28f81a7a32 100644
> > --- a/gcc/jit/libgccjit.map
> > +++ b/gcc/jit/libgccjit.map
> > @@ -186,4 +186,10 @@ LIBGCCJIT_ABI_13 {
> > gcc_jit_version_major;
> > gcc_jit_version_minor;
> > gcc_jit_version_patchlevel;
> > -} LIBGCCJIT_ABI_12;
> > \ No newline at end of file
> > +} LIBGCCJIT_ABI_12;
> > +
> > +LIBGCCJIT_ABI_14 {
> > + global:
> > + gcc_jit_function_get_return_type;
> > + gcc_jit_function_get_param_count;
> > +} LIBGCCJIT_ABI_13;
>
> Likewise.
>
> [...]
>
> Otherwise looks good to me.
>
> Bonus points for adding C++ bindings (and docs for them), but I don't
> know of anyone using the C++ bindings.
Also, please add "PR jit/96889" to the ChangeLog entries, and [PR96889]
to the subject line.
Dave
