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

From: Jeff King <peff@peff.net>
Date: Mar 12, 2013, 19:04 UTC
Message-ID: <20130312190456.GC17099@sigill.intra.peff.net>
In-Reply-To: <20130312154434.GB3021@sandbox-ub.fritz.box>

On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:

Show 11 quoted lines

> > 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.
   */

Show 5 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.

I do not care that much in this instance, since we have already discussed it, and I know what is going on, though.

-Peff