Re: [PATCH v2 1/4] config: factor out config file stack management
- From
Heiko Voigt <hvoigt@hvoigt.net>
- Date
- Mar 14, 2013, 06:36 UTC
- Message-ID
- <20130314063629.GA4062@sandbox-ub.fritz.box>
- In-Reply-To
- <20130312190456.GC17099@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 03:04:56PM -0400, Jeff King wrote:
Show 20 quoted lines
> On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote: > > > > Can we throw in a comment at the top here with the expected usage? In > > > particular, do_config_from is expecting the caller to have filled in > > > certain fields (at this point, top->f and top->name), but there is > > > nothing to make that clear. > > > > Of course. Will do that in the next iteration. How about I squash this in: > > [...] > > +/* The fields data, name and the source specific callbacks of top need > > + * to be initialized before calling this function. > > + */ > > static int do_config_from_source(struct config_source *top, config_fn_t fn, voi > > I think that is OK, but it may be even better to list the fields by > name. Also, our multi-line comment style is: > > /* > * Multi-line comment. > */
Ok will do both.
Show 10 quoted lines
> > I would add that to the third patch: > > > > config: make parsing stack struct independent from actual data source > > > > because that contains the final modification to config_file/config_source. > > It does not matter to the end result, but I find it helps with reviewing > when the comment is added along with the function, and then expanded as > the function is changed. It helps to understand the effects of later > patches if they need to tweak comments.
To make the series more clear to others who read it later, I will add the comment from the beginning.
Cheers Heiko