Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source

[Heiko Voigt <hvoigt@hvoigt.net>]

On Tue, Mar 12, 2013 at 07:03:55AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:
> 
> > diff --git a/config.c b/config.c
> > index f55c43d..fe1c0e5 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -10,20 +10,42 @@
> >  #include "strbuf.h"
> >  #include "quote.h"
> >  
> > -typedef struct config_file {
> > -	struct config_file *prev;
> > -	FILE *f;
> > +struct config_source {
> > +	struct config_source *prev;
> > +	void *data;
> 
> Would a union be more appropriate here? We do not ever have to pass it
> directly as a parameter, since we pass the "struct config_source" to the
> method functions.
> 
> It's still possible to screw up using a union, but it's slightly harder
> than screwing up using a void pointer. And I do not think we need the
> run-time flexibility offered by the void pointer in this case.

No we do not need the void pointer flexibility. But that means every new
source would need to add to this union. Junio complained about that fact
when I first added the extra members directly to the struct. A union
does not waste that much space and we get some seperation using the
union members. Since this struct is local only I think that should be
ok.

> > +static int config_file_fgetc(struct config_source *conf)
> > +{
> > +	FILE *f = conf->data;
> > +	return fgetc(f);
> > +}
> 
> This could become just:
> 
>   return fgetc(conf->u.f);
> 
> and so forth (might it make sense to give "f" a more descriptive name,
> as we are adding other sources?).

Will change that.

Cheers Heiko