On Wed, 21 Jul 2021 02:35:56 +0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxx> wrote: > On Tue, Jul 20, 2021 at 2:04 AM Haoran Luo <www@xxxxxxxxxxxxxx> wrote: > > > > return reader->read == rb_page_commit(reader) && > > (commit == reader || > > (commit == head && > > - head->read == rb_page_commit(commit))); > > + (rb_page_commit(commit) == 0 || > > + head->read == rb_page_commit(commit)))); > > } > > Can we please re-write that whole thing to be legible? > > That conditional was complicated before. It's worse now. > > For example, just splitting it up would help: > > if (reader->read != rb_page_commit(reader)) > return false; > > if (commit == reader) > return true; > > if (commit != head) > return false; > > return !rb_page_commit(commit) || > commit->read == rb_page_commit(commit); > > but adding comments to each conditional case would also be good. > > Note: I checked "head->read" to "commit->read" in that last > conditional, because we had _just_ verified that "commit" and "head" > are the same, and it seems to be more logical to check "commit->read" > with "rb_page_commit(commit)". > > (And somebody should check that I split it right - I'm just doing this > in the MUA without knowing the code: I hate conditionals that are > cross multiple lines like this, and I hate them particularly when they > turn out to be buggy, so when you fix a complex conditional, that's a > BIG sign to me that the problem was that the conditional was too > complex to begin with). > > Btw, that "rb_page_commit()" forces a new atomic read each time, so > the "rb_page_commit(commit)" value should probably be cached as a > variable before it's used twice. > > Steven? > > Linus > Agreed, this piece of code would be less likely to be buggy if they are commented by what they are judging. I will do it and send the next version of patch.
|