On Tue, Sep 22, 2015 at 08:22:39AM -0400, Brian Foster wrote:
> On Tue, Sep 22, 2015 at 09:54:32AM +0200, Carlos Maiolino wrote:
> > On Tue, Sep 22, 2015 at 08:08:32AM +1000, Dave Chinner wrote:
> > > On Mon, Sep 21, 2015 at 10:56:08AM +0200, Carlos Maiolino wrote:
> ...
> > > > +
> > > > + bulkreq.lastip = &last;
> > > > + bulkreq.icount = 1;
> > > > + bulkreq.ubuffer = &igroup;
> > > > + bulkreq.ocount = &count;
> > > > +
> > > > + if (!xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq)) {
> > > > + if (count > 0) {
> > > > + printf("Filesystem does have 64bit inodes\n");
> > > > + return 0;
> > > > + } else {
> > > > + printf("Filesystem does not have 64bit inodes\n");
> > > > + return 0;
> > > > + }
> > > > + }
> > > > + perror("xfsctl(XFS_IOC_FSINUMBERS)");
> > >
> > > I'd do this the other way around:
> > >
> > > if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq)) {
> > > perror("XFS_IOC_FSINUMBERS");
> > > exitcode = 1;
> > > return 0;
> > > }
> > >
> > > if (count > 0)
> > > printf("Filesystem does have 64bit inodes\n");
> > > else
> > > printf("Filesystem does not have 64bit inodes\n");
> > > return 0;
> > >
> > > is sufficient, xfsctl is a one line wrapper around ioctl().
> > >
> >
> > Ok, I'll change this, but, just a matter of curiosity, what are the technical
> > reasons to write the conditional this way, instead of the way I wrote first?
> > Just to avoid entering the conditional?
> >
>
> Not to speak for Dave, but I had the same thought just skimming through
> the patch. I don't think it's a technical reason so much as a style one.
> The further the function is grown, the easier it is to see that this
> kind of flow is not the most friendly thing. E.g.:
>
> ret = do_stuff();
> if (!ret) {
> ...
> ret = do_more_stuff();
> if (!ret) {
> ...
> }
> }
>
> In other words, the common execution path of the function starts to flow
> "inward" rather than the natural top-down flow of the function:
>
> ret = do_stuff();
> if (ret)
> handle_err;
>
> ret = do_more_stuff();
> if (ret)
> handle_err;
> ...
>
> The latter tends to be easier to follow, easier to review the error
> handling cases, probably avoids indentation problems down the road, etc.
> The function as it is right now is simple and so this probably isn't a
> big deal, but I suspect seeing the latter form so frequently in all of
> our other code is probably what causes something like the former to
> stand out (even in a simple/harmless example). Just my .02. :)
That's pretty much it from a code maintenance POV.
Also, though, consider how the compiler optimises code - the code
inside an if () condition usually involves resolving a branch
due to a jump statement to somewhere else. IOWs, your original code
might end up looking something like this:
if (condition)
goto L1
<code>
ret:
return
L1:
if (condition)
goto L2
<code>
goto ret;
L2:
if (condition)
goto L3
<code>
goto ret;
L3:
.....
and so a series of nested conditions results in lots of branches
beign taken during the fast path execution. That leads to a
larger CPU instruction cache footprint that the code generated from
the latter case, which looks like:
if (!condition)
goto L1
if (!condition)
goto L2
if (!condition)
goto L3
<code>
ret:
return
L1:
<code>
goto ret;
L2
<code>
goto ret;
L3:
.....
See the difference in the fast path execution? It's compact and no
branches are taken anywhere and so has a smaller instruction cache
footprint. And less instruction cache misses mean faster code
execution.
Hence on top of the code is being easier to read, modify and
maintain, and it's also much easier for the compiler to optimise
into fast, efficient code....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
