Hi Vegard,
(I'm cc'ing Eduard on the args map question.)
On Sat, 2009-08-22 at 23:39 +0200, Vegard Nossum wrote:
> @@ -371,7 +371,56 @@ int vm_class_link(struct vm_class *vmc, const
> struct cafebabe_class *class)
> }
> }
>
> - vmc->methods = malloc(sizeof(*vmc->methods) * class->methods_count);
> + unsigned int nr_miranda_methods = 0;
> + struct vm_method **miranda_methods = NULL;
> +
> + /* If in any of the superinterfaces we find a method which is not
> + * defined in this class file, we need to add a "miranda" method.
> + * Note that we don't need to do this recursively for all super-
> + * interfaces because they will have already done this very same
> + * procedure themselves. */
> + for (unsigned int i = 0; i < class->interfaces_count; ++i) {
> + struct vm_class *vmi = vmc->interfaces[i];
> +
> + for (unsigned int j = 0; j < vmi->nr_methods; ++j) {
> + struct vm_method *vmm = &vmi->methods[j];
> +
> + assert(vmm->name);
> + assert(vmm->type);
The assertions look useless. Why do we have them here?
> +
> + /* We need this "manual" recursive lookup because we
> + * haven't initialized this class' list of methods
> + * yet... */
> + unsigned int index = 0;
> + if (!cafebabe_class_get_method(vmc->class,
> + vmm->name, vmm->type, &index))
> + {
Too deep nesting here? The brace is on the wrong line and you might want
to fix the cafebabe API _not_ to return index in arguments and probably.
> + continue;
> + }
> +
> + if (!vmc->super)
> + continue;
> +
> + if (vm_class_get_method_recursive(vmc->super,
> + vmm->name, vmm->type))
> + {
Opening brace on the wrong line.
> + continue;
> + }
> +
> + /* XXX: Use generic resizable array? */
Hey, either fix it or drop the annoying XXX comment. There's obviously
nothing wrong with using realloc() here except that it's buggy and
doesn't check for NULL.
> + miranda_methods = realloc(miranda_methods,
> + sizeof(*miranda_methods)
> + * (nr_miranda_methods + 1));
> +
> + miranda_methods[nr_miranda_methods] = vmm;
> +
> + ++nr_miranda_methods;
> + }
> + }
> +
> + vmc->nr_methods = class->methods_count + nr_miranda_methods;
> +
> + vmc->methods = malloc(sizeof(*vmc->methods) * vmc->nr_methods);
> if (!vmc->methods) {
> NOT_IMPLEMENTED;
> return -1;
> @@ -387,7 +436,28 @@ int vm_class_link(struct vm_class *vmc, const struct
> cafebabe_class *class)
>
> vmm->itable_index = itable_hash(vmm);
>
> - if (vm_method_prepare_jit(&vmc->methods[i])) {
> + /* XXX: Only if the method actually has code. */
Hmm? What does this mean?
> + if (vm_method_prepare_jit(vmm)) {
> + NOT_IMPLEMENTED;
NOT_IMPLEMENTED is banned from new code.
> + return -1;
> + }
> + }
> +
> + for (uint16_t i = 0; i < nr_miranda_methods; ++i) {
> + struct vm_method *vmm = &vmc->methods[class->methods_count + i];
> +
> + if (vm_method_init_from_interface(vmm, vmc,
> + class->methods_count + i, miranda_methods[i]))
> + {
Opening brace on the wrong line.
> + NOT_IMPLEMENTED;
Banned from new code.
> + return -1;
> + }
> +
> + /* Share with above? */
> + vmm->itable_index = itable_hash(vmm);
> +
> + /* XXX: We _know_ that this method doesn't have any code. */
This is the third or fourth XXX added by this patch. Needless to say,
it's annoying.
> + if (vm_method_prepare_jit(vmm)) {
> NOT_IMPLEMENTED;
NOT_IMPLEMENTED.
> return -1;
> }
> @@ -829,9 +899,13 @@ struct vm_method *vm_class_get_method(const struct
> vm_class *vmc,
> if (vmc->kind != VM_CLASS_KIND_REGULAR)
> return NULL;
>
> - unsigned int index = 0;
> - if (!cafebabe_class_get_method(vmc->class, name, type, &index))
> - return &vmc->methods[index];
> + /* Could do binary search here if we wanted. */
You forgot XXX here. Seriously though, I'm not sure I see the value of
this comment. If it's a _known_ problem, it should be fixed and it's not
as if perf won't be able to find this call-site if it turns out to be a
problem later on.
> + for (unsigned int i = 0; i < vmc->nr_methods; ++i) {
> + struct vm_method *vmm = &vmc->methods[i];
> +
> + if (!strcmp(vmm->name, name) && !strcmp(vmm->type, type))
> + return vmm;
> + }
>
> return NULL;
> }
> diff --git a/vm/method.c b/vm/method.c
> index 095c1de..4be9c31 100644
> --- a/vm/method.c
> +++ b/vm/method.c
> @@ -141,6 +141,37 @@ int vm_method_init(struct vm_method *vmm,
> return 0;
> }
>
> +int vm_method_init_from_interface(struct vm_method *vmm, struct vm_class
> *vmc,
> + unsigned int method_index, struct vm_method *interface_method)
The indentation of the method arguments is really strange because
there's not much visual clue to separate the second line from actual
code.
> +{
> + /* NOTE: If we ever keep reference counts on loaded classes, we should
> + * perhaps _copy_ the interformation from the interface method instead
> + * of just grabbing a reference to the same information. */
> +
> + vmm->class = vmc;
> + vmm->method_index = method_index;
> + vmm->method = interface_method->method;
> +
> + vmm->name = interface_method->name;
> + vmm->type = interface_method->type;
> +
> + vmm->args_count = interface_method->args_count;
> + vmm->is_vm_native = false;
> +
> + /* XXX: Do we need to duplicate args_map? */
AFAICT, yes. Eduard?
> + /* All interface methods are abstract. */
> + {
What's with the braces?
> + vmm->code_attribute.max_stack = 0;
> + vmm->code_attribute.max_locals = vmm->args_count;
> +
> + vmm->line_number_table_attribute.line_number_table_length = 0;
> + vmm->line_number_table_attribute.line_number_table = NULL;
> + }
> +
> + return 0;
> +}
> +
> int vm_method_prepare_jit(struct vm_method *vmm)
> {
> struct compilation_unit *cu;
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
_______________________________________________
Jatovm-devel mailing list
Jatovm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jatovm-devel
