Ha, disregard, caught up to read the other email and I see everyone else got
there too. I would've been in the thread an hour ago, but unfortunately timed
call back from Microsoft support, much to my dismay.
Carry on then...
-----Original Message-----
From: xfs-bounces@xxxxxxxxxxx [mailto:xfs-bounces@xxxxxxxxxxx] On Behalf Of
Shaun Gosse
Sent: Friday, February 28, 2014 3:33 PM
To: Mark Tinguely; Brian Foster
Cc: xfs@xxxxxxxxxxx
Subject: RE: [PATCH] xfs: check all buffers in xfs_check_page_type()
> Is there any reason to scan all the buffers when we all we want is an
> indication that at least one is acceptable?
> Maybe there are generally not may buffers to a page to make it worthwhile.
>
The problem is the else doesn't check if acceptable is set before breaking. So
it can quit early, not having found what it was looking for even though it was
in one of the later buffers. Pre-mature optimization...
So the patch is good as is. It improves correctness.
If the optimization were desired, though, it could be:
+ else if (acceptable)
+ break
In addition to the remove already posted, and with proper whitespace.
Fair warning: this is without looking at the code, just inferring from what's
been written in those two emails. I'm hearing my CS101 professor in my head and
thinking may not be worth a couple cycles at the risk of correctness or clarity
(and suggesting code blind seems risky).
Actually, this could be the first condition. If the whole point of that loop is
just to make sure to set acceptable if it can be set according to the rules on
the basis of at least one of the buffers, then a starting:
+ if (acceptable)
+ break
With an annotation something like "/** Optimization: Quit when acceptable has
been set. Loop has no other side-effects to worry about.*/" seems reasonable
(and hopefully is true). And then it's clear from the code that's the point of
the loop and how it works.
Cheers,
-Shaun
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
|