diff mbox

Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

Message ID CA+6hz4r4FjfTN5+AfM=x7_2xCC-3_FMeniMHdYNufBRrd4g8Lg@mail.gmail.com
State New
Headers show

Commit Message

Feng Gao July 7, 2015, 4:02 p.m. UTC 
Hi Siddhesh,

Thanks your response.
The attachment is the latest change according to your suggestion
including the ChangeLog change.

About the tests, I did the following cases:
1. Write test codes to check if the (_IO_LINE_BUF|_IO_UNBUFFERED)
equals  (_IO_LINE_BUF+_IO_UNBUFFERED);
2. Update the glibc to check if it works like before.

On Tue, Jul 7, 2015 at 3:31 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> Please describe the testing you have done to verify that your patch is
> correct.  In general for such monotonous changes, it is sufficient to
> verify that there is no change in the generated code or any change is
> expected and contained.  Also, run the testsuite to verify that no
> additional errors are introduced.
>
> Add a space between the operator; I know this was wrong before, but it
> should be corrected with this patch.  That is,
>
>     _IO_LINE_BUF|_IO_UNBUFFERED
>
> should be
>
>     _IO_LINE_BUF | _IO_UNBUFFERED
>
> Your patch also needs a ChangeLog entry.  The patch is probably
> trivial enough that it does not need a copyright assignment.
>
> Please post the patch with the suggested change, a ChangeLog entry and
> a description of how you tested the patch.
>
> Thanks,
> Siddhesh
>
> On Tue, Jun 30, 2015 at 07:23:57AM +0800, Feng Gao wrote:
>> Hi all,
>>
>> Both of "_IO_UNBUFFERED" and "_IO_LINE_BUF"  are the bit flags, but I
>> find there are some codes looks like "_IO_LINE_BUF+_IO_UNBUFFERED",
>> while some codes are "_IO_LINE_BUF|_IO_UNBUFFERED".
>>
>> I think the former is not good, even though the final result is same.
>>
>> The attachment is my patch .
>>
>> Thanks
>> Feng
>
>> diff --git a/libio/fileops.c b/libio/fileops.c
>> index 7c7fef1..93d0d94 100644
>> --- a/libio/fileops.c
>> +++ b/libio/fileops.c
>> @@ -541,7 +541,7 @@ new_do_write (fp, data, to_do)
>>    _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
>>    fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
>>    fp->_IO_write_end = (fp->_mode <= 0
>> -                    && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +                    && (fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>                      ? fp->_IO_buf_base : fp->_IO_buf_end);
>>    return count;
>>  }
>> @@ -881,7 +881,7 @@ _IO_new_file_overflow (f, ch)
>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>
>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>> -      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>       f->_IO_write_end = f->_IO_write_ptr;
>>      }
>>    if (ch == EOF)
>> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
>> index c68ca6a..343875a 100644
>> --- a/libio/oldfileops.c
>> +++ b/libio/oldfileops.c
>> @@ -313,7 +313,7 @@ old_do_write (fp, data, to_do)
>>      fp->_cur_column = _IO_adjust_column (fp->_cur_column - 1, data, count) + 1;
>>    _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
>>    fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
>> -  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>                      ? fp->_IO_buf_base : fp->_IO_buf_end);
>>    return count;
>>  }
>> @@ -418,7 +418,7 @@ _IO_old_file_overflow (f, ch)
>>        f->_IO_write_end = f->_IO_buf_end;
>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>
>> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>       f->_IO_write_end = f->_IO_write_ptr;
>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>>      }
>> diff --git a/libio/wfileops.c b/libio/wfileops.c
>> index 3f628bf..14332ac 100644
>> --- a/libio/wfileops.c
>> +++ b/libio/wfileops.c
>> @@ -106,7 +106,7 @@ _IO_wdo_write (fp, data, to_do)
>>            fp->_wide_data->_IO_buf_base);
>>    fp->_wide_data->_IO_write_base = fp->_wide_data->_IO_write_ptr
>>      = fp->_wide_data->_IO_buf_base;
>> -  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>                                  ? fp->_wide_data->_IO_buf_base
>>                                  : fp->_wide_data->_IO_buf_end);
>>
>> @@ -465,7 +465,7 @@ _IO_wfile_overflow (f, wch)
>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>
>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>       f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
>>      }
>>    if (wch == WEOF)
>

Comments

Siddhesh Poyarekar July 7, 2015, 4:28 p.m. UTC | #1 
On Wed, Jul 08, 2015 at 12:02:38AM +0800, Feng Gao wrote:
> About the tests, I did the following cases:
> 1. Write test codes to check if the (_IO_LINE_BUF|_IO_UNBUFFERED)
> equals  (_IO_LINE_BUF+_IO_UNBUFFERED);
> 2. Update the glibc to check if it works like before.

Thanks, it looks like you missed fixing spacing in the last instance
(quoted below).  Also, run 'make check' before and after the patch to
make sure that there are no regressions due to this change.  Updating
glibc is a brave thing to do - if it breaks, your box is a brick that
only a rescue disk can get back :)

Please post an updated patch and also let me know the results of your
test.

Thanks,
Siddhesh

> @@ -477,7 +477,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>  
>        f->_flags |= _IO_CURRENTLY_PUTTING;
> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  	f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
>      }
>    if (wch == WEOF)
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index ff64fdc..dcd1058 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@ 
+015-07-07  Feng Gao  <gfree.wind@gmail.com>
+	* libio/fileops.c: Use "|" instead of "+" when combine _IO_LINE_BUF
+	and _IO_UNBUFFERED
+	* libio/oldfileops.c: Likewise
+	* libio/wfileops.c: Likewise
+
 2015-07-07  Stefan Liebler  <stli@linux.vnet.ibm.com>
 
 	[BZ #18508]
diff --git a/libio/fileops.c b/libio/fileops.c
index 9668024..cbcd6f5 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -521,7 +521,7 @@  new_do_write (_IO_FILE *fp, const char *data, _IO_size_t to_do)
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
   fp->_IO_write_end = (fp->_mode <= 0
-		       && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+		       && (fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -844,7 +844,7 @@  _IO_new_file_overflow (_IO_FILE *f, int ch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
     }
   if (ch == EOF)
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 84939e3..54789b2 100644
--- a/libio/oldfileops.c
+++ b/libio/oldfileops.c
@@ -313,7 +313,7 @@  old_do_write (fp, data, to_do)
     fp->_cur_column = _IO_adjust_column (fp->_cur_column - 1, data, count) + 1;
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
-  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -418,7 +418,7 @@  _IO_old_file_overflow (f, ch)
       f->_IO_write_end = f->_IO_buf_end;
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
       f->_flags |= _IO_CURRENTLY_PUTTING;
     }
diff --git a/libio/wfileops.c b/libio/wfileops.c
index 73d7709..fd51f96 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -118,7 +118,7 @@  _IO_wdo_write (_IO_FILE *fp, const wchar_t *data, _IO_size_t to_do)
 	     fp->_wide_data->_IO_buf_base);
   fp->_wide_data->_IO_write_base = fp->_wide_data->_IO_write_ptr
     = fp->_wide_data->_IO_buf_base;
-  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 				   ? fp->_wide_data->_IO_buf_base
 				   : fp->_wide_data->_IO_buf_end);
 
@@ -216,7 +216,7 @@  _IO_wfile_underflow (_IO_FILE *fp)
 
   /* Flush all line buffered files before reading. */
   /* FIXME This can/should be moved to genops ?? */
-  if (fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
+  if (fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
     {
 #if 0
       _IO_flush_all_linebuffered ();
@@ -477,7 +477,7 @@  _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 	f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
     }
   if (wch == WEOF)