Re: dd PATCH: add conv=direct

[Jim Meyering <jim@meyering.net>]

Paul Eggert <eggert@CS.UCLA.EDU> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> 2004-04-08  Jim Meyering  <jim@meyering.net>
>>
>> 	* src/dd.c (set_fd_flags): Don't OR in -1 when fcntl fails.
>
> Doesn't that fix generate worse code in the usual case, since it
> causes two conditional branches instead of one?
>
> How about this further patch?  It relies on common subexpression
> elimination, but that's common these days.
>
> 2004-04-08  Paul Eggert  <eggert@twinsun.com>
>
> 	* src/dd.c (set_fd_flags): Don't test old_flags < 0 twice.

I see your point.  Thanks for the comment fix.
I've gone ahead and reverted my change,
since I think your original code is a little more
readable than the more-efficient-when-fcntl-fails code
that you're proposing.

	(set_fd_flags): Undo part of today's change: it's a little
	cleaner -- and more efficient in the common case -- to go
	ahead and OR in the -1 when fcntl fails.

Index: src/dd.c
===================================================================
RCS file: /fetish/cu/src/dd.c,v
retrieving revision 1.158
retrieving revision 1.159
diff -u -p -u -r1.158 -r1.159
--- a/src/dd.c	8 Apr 2004 21:26:28 -0000	1.158
+++ b/src/dd.c	8 Apr 2004 21:30:18 -0000	1.159
@@ -1014,7 +1014,7 @@ copy_with_unblock (char const *buf, size
 }
 
 /* Set the file descriptor flags for FD that correspond to the nonzero bits
-   in FLAGS.  The file's name is NAME.  */
+   in ADD_FLAGS.  The file's name is NAME.  */
 
 static void
 set_fd_flags (int fd, int add_flags, char const *name)
@@ -1022,7 +1022,7 @@ set_fd_flags (int fd, int add_flags, cha
   if (add_flags)
     {
       int old_flags = fcntl (fd, F_GETFL);
-      int new_flags = old_flags < 0 ? add_flags : (old_flags | add_flags);
+      int new_flags = old_flags | add_flags;
       if (old_flags < 0
 	  || (new_flags != old_flags && fcntl (fd, F_SETFL, new_flags) == -1))
 	error (EXIT_FAILURE, errno, _("setting flags for %s"), quote (name));