On Tue, Feb 26, 2013 at 02:10:03PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html