Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Mar 12, 2013, 16:00 UTC
Message-ID: <20130312160056.GB4472@sandbox-ub.fritz.box>
In-Reply-To: <20130312110003.GD11340@sigill.intra.peff.net>

On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:

Show 29 quoted lines

> On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
> 
> > The only location where cf is set in this file is in do_config_from().
> > This function has only one callsite which is config_from_file(). In
> > config_from_file() its ensured that the f member is set to non-zero.
> > [...]
> > -	if (cf && ((f = cf->f) != NULL)) {
> > +	if (cf) {
> 
> I still think we can drop this conditional entirely. The complete call
> graph looks like:
> 
>   git_config_from_file
>     -> git_parse_file
>       -> get_next_char
>       -> get_value
>           -> get_next_char
>           -> parse_value
>               -> get_next_char
>       -> get_base_var
>           -> get_next_char
>           -> get_extended_base_var
>               -> get_next_char
> 
> That is, every path to get_next_char happens while we are in
> git_config_from_file, and that function guarantees that cf = &top, and
> that top.f != NULL.  We do not have to even do any analysis of the
> conditions for each call, because we never change "cf" nor "top.f"
> except when we set them in git_config_from_file.

Ok if you say so I will do that :-). I was thinking about adding a patch that would remove cf as a global variable and explicitely pass it down to get_next_char. That makes it more obvious that it actually is != NULL. Looking at your callgraph I do not think its that much work. What do you think?

BTW, how did you generate this callgraph? Do you have a nice tool for that?

Cheers Heiko