Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source
- From
Heiko Voigt <hvoigt@hvoigt.net>
- Date
- Mar 12, 2013, 16:27 UTC
- Message-ID
- <20130312162734.GB4704@sandbox-ub.fritz.box>
- In-Reply-To
- <20130312110355.GE11340@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 07:03:55AM -0400, Jeff King wrote:
Show 24 quoted lines
> 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.
Show 12 quoted lines
> > +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