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

[Heiko Voigt <hvoigt@hvoigt.net>]

On Thu, Feb 28, 2013 at 01:41:47AM +0100, Heiko Voigt wrote:
> On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:
> > On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:
> > > On Tue, Feb 26, 2013 at 08:40:23PM +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.
> > > 
> > > Makes sense, although...
> > > 
> > > > -	if (cf && ((f = cf->f) != NULL)) {
> > > > +	if (cf) {
> > > > +		FILE *f = cf->f;
> > > 
> > > Couldn't we say the same thing about "cf" here (i.e., that it would
> > > never be NULL)? Can we just get rid of this conditional entirely?
> > 
> > That might be true. I will look into it. Just wanted to get rid of an
> > extra callback in my series.
> 
> I had a look and it might be true that cf will never be NULL in a code
> path. Nevertheless its much harder to verify by looking at the code
> since its a global variable. get_next_char() is called from all over the
> place and I would have to look at all the code paths. As far as I know
> static global variables are always initialized to zero so its safe to
> check even if has not yet been explicitly initialized.
> 
> The statement if cf is not NULL all members will be initialized is much
> simpler to verify since its just one place now and two places after this
> series.

To add some empirical information: I just ran the testsuite without the
conditional and it still passes. To me it only make sense to start the
parsing with cf initialized. But I am not familiar enough with the code
to judge whether it is safe to assume this.

Cheers Heiko