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
|