Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: local policy)
Message-ID: <46CB8D59.4080409@roguewave.com>
Date: Tue, 21 Aug 2007 19:11:53 -0600
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software, Inc.
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.6) Gecko/20070802 SeaMonkey/1.1.4
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: [PATCH] STDCXX-522
References: <e059dee80708171549lbd6ea2eo9f7d47af6134a18@mail.gmail.com>
	 <46C76ABC.5010104@roguewave.com>
 <e059dee80708181637p1fcda333v4a74846553d5665@mail.gmail.com>
In-Reply-To: <e059dee80708181637p1fcda333v4a74846553d5665@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Everton Araujo wrote:
> Sorry Martin, I didn't observe the coding style ... my bad. I'll change it
> right now.
> About the test, I think it's good for avoiding unnecessary xsputn calls
> considering that it just exists when char count (__nchar) is equals 0.

Perhaps it is. But if it's not necessary to fix the bug such
a change should be made in a separate patch to minimize the
scope of the bugfix. We try to highlight this guideline on
our bugs page but possibly not be as clearly as we should:
   http://incubator.apache.org/stdcxx/bugs.html#patch_format

> May I suggest something? (If not, jmp _end)

Yes! Please do! :) Suggestions for improvements are welcome!

> I think you would consider
> transform xsputn in an inline function which may call "_xsputn" (the full
> implementation) just when count greater than 0, automatically avoiding
> unnecessary calls. What do you think about it? Please remember, it's just a
> suggestion, don't hate me, give me a smile :-)

I assume you're proposing to add a new inline function that
would be used internally by our implementation (we can't change
the name xsputn() since it's a required interface). That might
be something to consider, although there already is a public
inline function called sputn() that presumably could be used
for this *if* the standard requirements on it were relaxed so
as to allow it to return without calling xsputn() when its
second argument is 0. The other issue with calling sputn()
from overflow() is that it would end up calling the xsputn()
defined in a derived class, which is not permitted by the
standard (that's why the call to xsputn() is qualified with
the name of basic_filebuf). So this will need some thought.

Thanks again for your suggestion and the patch. I'll test it
and commit it as soon as I've seen successful test results.

Martin

> _end:
> The test is not required for correct patch behavior. You may ignore it with
> no regards. :-)
> 
> Regards.
> 
> Everton.
> 
> 2007/8/18, Martin Sebor <sebor@roguewave.com>:
>> Everton Araujo wrote:
>>> Thank you Martin and Andrew for helping me.
>>>
>>> Below is the patch for STDCXX-522 (std::filebuf::overflow(EOF) writes
>> EOF to
>>> file in unbuffered mode)
>> Thanks for the patch! I have a couple of questions for you but first
>> let me make a general comment about our formatting style. The stdcxx
>> style guidelines haven't been migrated from Rogue Wave to Apache yet,
>> so until we have migrated them, contributors like you need to try to
>> figure them out by observing the existing code they are patching.
>> A couple of the basic ones are:
>>
>>    1. Use 4-space indents (no TABs).
>>    2. Separate every opening parenthesis, bracket, or curly brace
>>       from the preceding symbol by a single space.
>>
>> So by the way of example, ...
>>
>>> Index: include/fstream.cc
>>> ===================================================================
>>> --- include/fstream.cc    (revision 566470)
>>> +++ include/fstream.cc    (working copy)
>>> @@ -351,8 +351,15 @@
>>>          _RWSTD_STREAMSIZE __nchars;
>>>
>>>          if (__unbuf) {
>>> +          if(this->_C_is_eof(__c)){
>> ...this should be (note the two additional spaces before the if
>> and the one space after the if, after _C_is_eof, and before the
>> open curly brace):
>>
>>    +            if (this->_C_is_eof (__c)) {
>>
>> Now for the questions...
>>
>>> +            _C_cur_pos.state (0);
>>> +            __buf   = 0;
>>> +            __nchars = 0;
>>> +          }
>>> +          else{
>>>              __buf    = &__c_to_char;
>>>              __nchars = 1;
>>> +          }
>>>          }
>>>          else {
>>>              // call xsputn() with a special value to have it flush
>>> @@ -364,7 +371,7 @@
>>>          // typedef helps HP aCC 3.27
>>>          typedef basic_filebuf _FileBuf;
>>>
>>> -        if (__nchars != _FileBuf::xsputn (__buf, __nchars))
>>> +        if (__nchars && __nchars != _FileBuf::xsputn (__buf, __nchars))
>> Why is the extra test here necessary? I.f., why wouldn't the original
>> code be sufficient?
>>
>>>              return traits_type::eof ();  // error while writing
>>>      }
>>>
>>> @@ -424,7 +431,7 @@
>>>          typedef basic_filebuf _FileBuf;
>>>
>>>          // return -1 on error to flush the controlled sequence
>>> -        if (__nwrite != _FileBuf::xsputn (__special, __nwrite))
>>> +        if (__nwrite && __nwrite != _FileBuf::xsputn (__special,
>> __nwrite))
>>
>> Why is this change necessary at all?
>>
>> Martin
>>
>