Hi, Since errors are rare, it might be better to use an error code/boolean to signal success/failure, and keep the error info in a TLS variable.
Zoltan On Fri, Jun 26, 2009 at 6:35 PM, Paolo Molaro <lu...@ximian.com> wrote: > On 06/26/09 Rodrigo Kumpera wrote: > > The attached patch changes class.c/inflate_generic_type to not abort the > > runtime when facing a bad instantiation. > > > > My only issue is that I'm not sure if mono_class_inflate_generic_type* > and > > mono_class_inflate_generic_class should > > set a loader error when returning null. I don't think this is necessary > as > > this is something their callers should do. But maybe > > mono_class_inflate_generic_method_full should. > > In general only the low-level code knows the reason for the failure, so > the low-level code should propagate this info (and the exact reason for > the failure is often useful when debugging). Note how instead your patch > removes some potentially useful info. > For that reason I'm not much a fan of having functions return a boolean, > because that gives very little information. > On the other hand, using something like GError, with it's forced > malloc() usage may not be an appropriate pattern for use in the whole > runtime. > > A compromise suggestion could be something like this: > typedef struct { > unsigned short error_code; // this can be MONO_EXCEPTION_* > unsigned short flags; > void *data0; // same as exception_data in MonoClass or other > fields in MonoLoaderError > void *data1; > void *data2; > void *datan; > char message [128]; > } MonoError; > > A few considerations: > *) the struct would be passed by ref as the last argument of the runtime > functions > *) it can be stack allocated, so the malloc is avoided and it can be > used also in signals etc. This means that message should be kept small, > no more than 256, though. > *) eventually it should be also the mechanism for reporting errors with > the embedding interface, so there are a few dummy fields for future > expansion, but the internals should be considered private and once > published the struct won't be changed. > *) setting the error should be done with a few simple functions like: > > void mono_error_set_ok (MonoError *error); // no error > void mono_error_set_bad_image (MonoError *error, MonoImage *image, > char *msg_format, ...); > etc. > *) I think it would be a good idea to always require the error > argument to be non-null. > *) error checking would be done with the (macro equivalent) of: > gboolean mono_error_ok (MonoError *error); > > See below how the first snippet of code would become. > > > --- a/mono/metadata/class.c > > +++ b/mono/metadata/class.c > > @@ -490,20 +490,24 @@ mono_class_is_open_constructed_type (MonoType *t) > > } > > } > > > > -static MonoType* > > -inflate_generic_type (MonoImage *image, MonoType *type, > MonoGenericContext *context) > > +/* > > + * This function returns TRUE if it could proper inflate @type. > > + * The resulting type can be found in @res > > + */ > > +static gboolean > > +inflate_generic_type (MonoImage *image, MonoType *type, > MonoGenericContext *context, MonoType **res) > > { > > +#define SUCCESS(VAL) do { *res = VAL; return TRUE; } while (0) > > +#define ERROR() do { *res = NULL; return FALSE; } while (0) > > switch (type->type) { > > case MONO_TYPE_MVAR: { > > MonoType *nt; > > int num = mono_type_get_generic_param_num (type); > > MonoGenericInst *inst = context->method_inst; > > if (!inst || !inst->type_argv) > > - return NULL; > > + SUCCESS (NULL); > > if (num >= inst->type_argc) > > - g_error ("MVAR %d (%s) cannot be expanded in this > context with %d instantiations", > > - num, mono_generic_param_info > (type->data.generic_param)->name, inst->type_argc); > > - > > + ERROR (); > > static MonoType* > inflate_generic_type (MonoImage *image, MonoType *type, MonoGenericContext > *context, MonoError *error) > { > mono_error_set_ok (error); > switch (type->type) { > case MONO_TYPE_MVAR: { > MonoType *nt; > int num = mono_type_get_generic_param_num (type); > MonoGenericInst *inst = context->method_inst; > if (!inst || !inst->type_argv) > return NULL; > if (num >= inst->type_argc) > mono_error_set_bad_image (error, image, "MVAR %d > (%s) cannot be expanded in this context with %d instantiations", > num, mono_generic_param_info > (type->data.generic_param)->name, inst->type_argc); > return NULL; > ... > > Comments welcome (also by future users of the embedding interface). > > lupus > > -- > ----------------------------------------------------------------- > lu...@debian.org debian/rules > lu...@ximian.com Monkeys do it better > _______________________________________________ > Mono-devel-list mailing list > Mono-devel-list@lists.ximian.com > http://lists.ximian.com/mailman/listinfo/mono-devel-list >
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list