Re: [PATCH v3 06/34] Introduce interpreter factories

Simon Marchi
2016-05-18T19:18:06Z

On 16-05-06 08:34 AM, Pedro Alves wrote:
> +/* See interps.h. */
> +
> +struct interp *
> +interp_lookup (const char *name)
> +{
> + struct ui *ui = current_ui;
> + struct interp_factory *factory;
> + struct interp *interp;
> + int ix;
> +
> + if (name == NULL || strlen (name) == 0)
> + return NULL;
> +
> + /* Only create each interpreter once per top level. */
> + interp = interp_lookup_existing (name);
> + if (interp != NULL)
> + return interp;
> +
> + for (ix = 0;
> + VEC_iterate (interp_factory_p, interpreter_factories, ix, factory);
> + ++ix)
> + if (strcmp (factory->name, name) == 0)
> + {
> + interp = factory->func (name, ui);
> + interp_add (interp);
> + return interp;
> + }
> +
> + return NULL;
> +}
> +

I think there are some opportunities to reduce the number of functions that directly
access global state. For example, I think it would be clearer if interp_lookup took
a struct ui* as argument instead of accessing current_ui. It would show that it's used
to lookup an interpreter by name in a ui. Similarly, it calls interp_lookup_existing and
interp_add, which both use get_current_interp_info(), which is essentially accessing
current_ui. I believe it would be easier to follow if some caller provided the struct ui
and it was passed down by parameters.