Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()
- From
Heiko Voigt <hvoigt@hvoigt.net>
- Date
- Feb 28, 2013, 00:54 UTC
- Message-ID
- <20130228005432.GA23046@sandbox-ub>
- In-Reply-To
- <20130228004137.GA12948@sandbox-ub>
On Thu, Feb 28, 2013 at 01:41:47AM +0100, Heiko Voigt wrote:
Show 30 quoted lines
> 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