On Fri, Nov 7, 2014 at 9:21 AM, Roger Willcocks [off-list ref] wrote:

On Fri, 2014-11-07 at 08:43 +0200, Anton Altaparmakov wrote:

quoted

Hi,

quoted

On 7 Nov 2014, at 07:52, Anand Avati [off-list ref] wrote:
On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov [off-list ref] wrote:

quoted

On 7 Nov 2014, at 01:46, Jeff Moyer [off-list ref] wrote:
Minor nit, but I'd rather read something that looks like this:

      if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;
      else if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

But your version is less logically efficient for the case where "type == READ" is true and "flags & RWF_NONBLOCK" is false because your version then has to do the "if (type == WRITE" check before discovering it does not need to take that branch either, whilst the original version does not have to do such a test at all.

Seriously?

Of course seriously.

quoted

Just focus on the code readability/maintainability which makes the code most easily understood/obvious to a new pair of eyes, and leave such micro-optimizations to the compiler..

The original version is more readable (IMO) and this is not a micro-optimization.  It is people like you who are responsible for the fact that we need faster and faster computers to cope with the inefficient/poor code being written more and more...

Your original version needs me to know that type can only be either READ
or WRITE (and not, for instance, READONLY or READWRITE or some other
random special case) and it rings alarm bells when I first see it. If
you want to keep the micro optimization, you need an assertion to
acknowledge the potential bug and a comment to make the code obvious:

 +            assert(type == READ || type == WRITE);
 +            if (type == READ) {
 +                    if (flags & RWF_NONBLOCK)
 +                            return -EAGAIN;
 +            } else { /* WRITE */
 +                    if (flags & RWF_DSYNC)
 +                            return -EINVAL;
 +            }

but since what's really happening here is two separate and independent
error checks, Jeff's version is still better, even if it does take an
extra couple of nanoseconds.

Actually I'd probably write:

       if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;

       if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

(no 'else' since the code will never be reached if the first test is
true).


--
Roger Willcocks [off-list ref]

This is what I changed it to (and will be sending that out for the
next version).

-- 
Milosz Tanski
CTO
16 East 34th Street, 15th floor
New York, NY 10016

p: 646-253-9055
e: milosz@adfin.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>