On Sat, 3 Apr 2010 06:25:08 +0800
Bob Liu <lliubbo@xxxxxxxxx> wrote:
> >> - /*
> >> - * When checking the active state, we need to be sure we are
> >> - * dealing with comparible boolean values. Take the logical not
> >> - * of each.
> >> - */
> >
> > You deleted a spelling mistake too!
> >
> >> - if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
> >> - return ret;
> >> -
> >> - if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
> >> - return ret;
> >> + if (mode != ISOLATE_BOTH) {
> >> + if ((PageActive(page) != mode) ||
> >> + (page_is_file_cache(page) != file))
> >> + return ret;
> >> + }
> >
> > The compiler should be able to avoid testing for ISOLATE_BOTH twice,
>
> Thanks for your kindly reply.
> then is the two "not" able to avoid by the compiler ?
> if yes, this patch is meanless and should be ignore.
I very much doubt if the compiler knows that these two variables can
only ever have values 0 or 1, so I expect that removing the !'s will
reduce text size.
That being said, it wouldn't be a good and maintainable change -
one point in using enumerations such as ISOLATE_* is to hide their real
values. Adding code which implicitly "knows" that a particular
enumerated identifier has a particular underlying value is rather
grubby and fragile.
But the code's already doing that.
It's also a bit fragile to assume that a true/false-returning C
function (PageActive) will always return 0 or 1. It's a common C idiom
for such functions to return 0 or non-zero (not necessarily 1).
So a clean and maintainable implementation of
if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
return ret;
would be
if (mode != ISOLATE_BOTH &&
((PageActive(page) && mode == ISOLATE_ACTIVE) ||
(!PageActive(page) && mode == ISOLATE_INACTIVE)))
return ret;
which is just dying for an optimisation trick such as the one which is
already there ;)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>