On Tue, Mar 12, 2013 at 03:04:56PM -0400, Jeff King wrote: > 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. > > 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 -- 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