[vmkit-commits] [PATCH] Refactor Class-related code into new Class.inc.

[Will Dietz]

On Tue, Nov 8, 2011 at 2:36 PM, Nicolas Geoffray
<nicolas.geoffray at gmail.com> wrote:
> Looks good, with one request.
>> +// Implementation-specific logic
>> +JavaObject* ClassLib_getConstructor(JavaMethod * cons, int i);
>> +JavaObject* ClassLib_getMethod(JavaMethod* meth, int i);
>> +JavaObject* ClassLib_getField(JavaField* field, int i);
>> +
>> +static inline ArrayObject* Class_getDeclaredConstructors(JavaObject*
>> Cl, jboolean publicOnly) {
>
> Please put this method as a static method of JavaObjectClass (I know, you
> have to add it to the file in gnu classpath, as well as openjdk).
> The reason I'm asking is because I just realized that you're using static
> inline. Note that if it was static only, the GC would not work with it
> (limitation in clang). Because you're putting inline, I guess that's the
> reason why it did not fail, but that's very brittle. Please do the same for
> all the other methods above. Thanks!
>

Sure thing :).  I put it as static inline to avoid any potential
issues like that, but not because I actually ran into any.  In
particular, as code gets shared into these .inc files, having them
static inline seemed like it would help for things that wanted to walk
the stack, for example.  Apparently I got lucky regarding the issue
with clang/GC you mentioned :).

Anyway, I think I understand your proposed change, just want to
confirm: go with the original patch set (that had two separate
definitions of each of these) but instead of the code living in
OpenJDK.inc and ClasspathVMClass.inc, put them into each runtime
library's ClasspathReflect.h?

Additionally, would they need to be inline-friendly (which suggests
the entirety of their definition goes into ClasspathReflect.h), or can
I just declare them in ClasspathReflect.h and define them in
ClasspathReflect.cpp?

I don't mean to be difficult or dense, but it sounds like you had a
particular organization in mind (which seems doubly important given
that some layouts apparently don't play nice with the GC), and I want
to make sure I match it :).  Implementation is just a straightforward
refactoring once I'm on the same page as you :).

Thanks!

~Will