Messages in this thread |  | | | Date | Fri, 10 Feb 2006 09:21:04 +0900 | | From | Tejun Heo <> | | Subject | Re: [PATCH] block: kill not-so-popular simple flag testing macros |
| |
Jeff Garzik wrote: > Tejun Heo wrote: > >> This patch kills the following request flag testing macros. >> >> blk_noretry_request() >> blk_rq_started() >> blk_pm_suspend_request() >> blk_pm_resume_request() >> blk_sorted_rq() >> blk_barrier_rq() >> blk_fua_rq() >> >> All above macros test for single request flag and not used widely and >> seem to contribute more to obscurity than readability. >> >> Signed-off-by: Tejun Heo <htejun@gmail.com> > > > heh, I guess that's a manner of opinion :) > > To me, your patch makes things less readable. Example: > >> - int is_barrier = blk_fs_request(rq) && blk_barrier_rq(rq); >> + int is_barrier = blk_fs_request(rq) && rq->flags & REQ_HARDBARRIER; > > > After your change is applied, the above statement is no longer visually > consistent with itself. The reader must decode two different styles of > test in his brain, as opposed to one. >
Hello, Jeff.
Yeah, I didn't like that line either, but the thing that triggered this patch is this message from Linus. This is a reply to Andrew's dropped-from-rc message so not on lkml.
> I'm applying this under protest. > > The code may be right, but it visually makes _zero_ sense. > > The fact that "blk_barrier_rq()" only triggers on hard barriers means that > it does what you describe in the changelog, but read the code and see how > little sense it makes when you _read_ it. You just checked that the rq has > a barrier (either a soft or a hard one), and now you check "again" whether > it has a barrier. > > That's what it looks like from reading the code. Confusing. And thus prone > to bugs. >
The code he was talking about looks like.
if (rq->flags & (REQ_SOFTBARRIER | REQ_HARDBARRIER) { .... if (blk_barrier_rq(rq)) q->ordcolor ^= 1; .... } We could add blk_barrier_rq() to test for both flags and add blk_hardbarrier_rq() and blk_softbarrier_rq(), but the flags are not used that widely and some other flag testing macros are used only few times in core block layer, so I determined to kill infrequently used macros.
I agree that the change makes code harder to eyes in some places, but it doesn't obfuscate the code and results in less maintenance overhead in general. Remember a few testing macros is okay but remembering a dozen gets a bit annoying, IMHO.
Thanks.
-- tejun - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |