Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Sep 04, 2008 at 05:36:17PM -0400, Chuck Lever wrote:
> On Thu, Sep 4, 2008 at 4:23 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > Slightly irrelevant, but same comment as before--wouldn't it be easier
> > to follow the logic if instead of:
> >
> >        p = kstrndup(...)
> >        if (p) {
> >                do stuff for successful case
> >                ....
> >                return 1;
> >        }
> >
> >        return 0;
> >
> > it were:
> >
> >        p = kstrndup(...)
> >        if (!p)
> >                return 0;
> >        do stuff for successful case
> >        ...
> >        return 1;
> 
> I tried that.  It made this patch about 3x larger than it needed to
> be, obscured the changes introduced by the patch, and made it harder
> to show that the new logic is correct.  As we are not introducing new
> code here but repairing existing issues, such a clean up would need to
> be in a separate patch.

I agree.

> I generally prefer to use a structured language as it was intended,
> rather than to abuse "goto" as is the trend in the Linux kernel.

That's a well-established practice, not a trend, and in any case I
didn't add a goto.

> By creating named subroutines instead of using lambda functions

I don't see any difference over named subroutines here?  Does c even
support lambda functions?

To me the second example is obviously more readable, partly since it
helps clear up the return convention ("oh, we're returning 0 on failure
of kstrndup!  0 must mean failure..."), while the 1st delays one of the
two branches of the if longer than necessary.

But I don't care enough to insist.  Take it as an indicator of what'd be
clearer to me for code I have to read a lot, if you like.

--b.