Re: [PATCH 1/4] config: factor out config file stack management

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Feb 27, 2013, 07:51 UTC
Message-ID: <20130227075100.GA3268@sandbox-ub>
In-Reply-To: <7vobf6loas.fsf@alter.siamese.dyndns.org>

On Tue, Feb 26, 2013 at 02:10:03PM -0800, Junio C Hamano wrote:

Show 31 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I wonder if it would be more obvious with the more usual OO-struct
> > functions, like:
> >
> >   struct config_source {
> >           ...
> >   };
> >   void config_source_init_file(struct config_source *, const char *fn);
> >   void config_source_init_strbuf(struct config_source *,
> >                                  const struct strbuf *buf);
> >   void config_source_clear(struct config_source *);
> >
> >   int config_source_parse(struct config_source *);
> >
> > and then the use would be something like:
> >
> >   struct config_source top;
> >   int ret;
> >
> >   config_source_init_file(&top, "foo");
> >   ret = config_source_parse(&top);
> >   config_source_clear(&top);
> >
> >   return ret;
> >
> > I.e., "init" constructors, a "clear" destructor, and any methods like
> > "parse" that you need.
> 
> Yup, that cocincides with my first impression I sent out for the
> previous RFC/PATCH round.

I agree that your suggestions would make the design more OO and provide us with more flexibility. However at the following costs:

Currently the push and pop are combined into one function. This design makes it impossible to forget the cleanup if someone else uses this function. The same applies to the private data handling around do_config_from().

All memory is currently handled on the stack. If we have an init/clear design at least the private data for strbuf needs to be put on the heap. We could pass in the config_strbuf but IMO that would violate the encapsulation.

Thus we also require a specialized clear which frees that private data (we need that to close the file anyway). Because of that I would probably call it destroy or free.

That means there will be six additional functions instead of one: We need init and clear for both, a common init and a common clear for the push/pop part. IMO that will make the code harder to read for the first time.

Then we would have a hybrid stack/heap solution still involving side effects (setting the global cf).

Do not get me wrong. I am not against changing it but I would like to spell out the consequences first before doing so.

Do you still think that is nicer approach?

Cheers Heiko