Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > Thoughts on function

Reply
Thread Tools

Thoughts on function

 
 
Ebenezer
Guest
Posts: n/a
 
      09-15-2011

Shalom

How to improve this function?

bool
SendBuffer::Flush ()
{
uint32_t const all = buf_.size();
uint32_t numWritten = 0;
if (partialFlush_) {
::neolib::segmented_array<unsigned char, chunk_size>::segment&
segment =
segmented_iterator<unsigned char,
chunk_size>(buf_.begin()).segment();

numWritten = Write(sock_, &buf_[0], segment.size());
if (numWritten < segment.size()) goto ending;
partialFlush_ = false;
}

{
int32_t const chunks = (all - numWritten) / chunk_size;
for (int32_t ii = 0; ii < chunks; ++ii) {
uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
numWritten += bytes;
if (bytes < chunk_size) goto ending;
}
}

if (numWritten < all) {
numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
}

ending:
buf_.erase(buf_.begin(), buf_.begin() + numWritten);
if (numWritten == all) return true;

partialFlush_ = true;
return false;
}

Tia.

Brian Wood
Ebenezer Enterprises
http://webEbenezer.net
 
Reply With Quote
 
 
 
 
Ian Collins
Guest
Posts: n/a
 
      09-15-2011
On 09/15/11 05:01 PM, Ebenezer wrote:
>
> Shalom
>
> How to improve this function?
>
> bool
> SendBuffer::Flush ()
> {
> uint32_t const all = buf_.size();
> uint32_t numWritten = 0;


You're probably better off using size_t unless you have a really really
good reason to use uint32_t.

> if (partialFlush_) {
> ::neolib::segmented_array<unsigned char, chunk_size>::segment&
> segment =
> segmented_iterator<unsigned char,
> chunk_size>(buf_.begin()).segment();
>
> numWritten = Write(sock_,&buf_[0], segment.size());


Undeclared function again.

> if (numWritten< segment.size()) goto ending;


Ug.

> partialFlush_ = false;


Lots of undefined stuff there, so can't comment.

> }
>
> {
> int32_t const chunks = (all - numWritten) / chunk_size;
> for (int32_t ii = 0; ii< chunks; ++ii) {
> uint32_t bytes = Write(sock_,&buf_[numWritten], chunk_size);
> numWritten += bytes;
> if (bytes< chunk_size) goto ending;


Ug, why on earth did you go and spoil it by using goto?

--
Ian Collins
 
Reply With Quote
 
 
 
 
Ebenezer
Guest
Posts: n/a
 
      09-15-2011
On Sep 15, 12:08*am, Ian Collins <(E-Mail Removed)> wrote:
> On 09/15/11 05:01 PM, Ebenezer wrote:
>
>
>
> > Shalom

>
> > How to improve this function?

>
> > bool
> > SendBuffer::Flush ()
> > {
> > * *uint32_t const all = buf_.size();
> > * *uint32_t numWritten = 0;

>
> You're probably better off using size_t unless you have a really really
> good reason to use uint32_t.
>
> > * *if (partialFlush_) {
> > * * *::neolib::segmented_array<unsigned char, chunk_size>::segment&
> > segment =
> > * * * * * * * * segmented_iterator<unsigned char,
> > chunk_size>(buf_.begin()).segment();

>
> > * * *numWritten = Write(sock_,&buf_[0], segment.size());

>
> Undeclared function again.
>
> > * * *if (numWritten< *segment.size()) goto ending;

>
> Ug.
>
> > * * *partialFlush_ = false;

>
> Lots of undefined stuff there, so can't comment.


Sorry. Some are here --
http://webEbenezer.net/misc/SendBuffer.hh .
The Write function is here
http://webEbenezer.net/misc/IO.cc .

 
Reply With Quote
 
Ian Collins
Guest
Posts: n/a
 
      09-15-2011
On 09/16/11 02:34 AM, Ebenezer wrote:
> On Sep 15, 12:08 am, Ian Collins<(E-Mail Removed)> wrote:
>> On 09/15/11 05:01 PM, Ebenezer wrote:
>>
>>
>>
>>> Shalom

>>
>>> How to improve this function?

>>
>>> bool
>>> SendBuffer::Flush ()
>>> {
>>> uint32_t const all = buf_.size();
>>> uint32_t numWritten = 0;

>>
>> You're probably better off using size_t unless you have a really really
>> good reason to use uint32_t.
>>
>>> if (partialFlush_) {
>>> ::neolib::segmented_array<unsigned char, chunk_size>::segment&
>>> segment =
>>> segmented_iterator<unsigned char,
>>> chunk_size>(buf_.begin()).segment();

>>
>>> numWritten = Write(sock_,&buf_[0], segment.size());

>>
>> Undeclared function again.
>>
>>> if (numWritten< segment.size()) goto ending;

>>
>> Ug.
>>
>>> partialFlush_ = false;

>>
>> Lots of undefined stuff there, so can't comment.

>
> Sorry. Some are here --
> http://webEbenezer.net/misc/SendBuffer.hh .
> The Write function is here
> http://webEbenezer.net/misc/IO.cc .


But that doesn't excuse the gratuitous use of goto!

--
Ian Collins
 
Reply With Quote
 
Ebenezer
Guest
Posts: n/a
 
      09-16-2011
On Sep 15, 2:38*pm, Ian Collins <(E-Mail Removed)> wrote:
> On 09/16/11 02:34 AM, Ebenezer wrote:
>
>
>
>
>
>
>
>
>
> > On Sep 15, 12:08 am, Ian Collins<(E-Mail Removed)> *wrote:
> >> On 09/15/11 05:01 PM, Ebenezer wrote:

>
> >>> Shalom

>
> >>> How to improve this function?

>
> >>> bool
> >>> SendBuffer::Flush ()
> >>> {
> >>> * * uint32_t const all = buf_.size();
> >>> * * uint32_t numWritten = 0;

>
> >> You're probably better off using size_t unless you have a really really
> >> good reason to use uint32_t.

>
> >>> * * if (partialFlush_) {
> >>> * * * ::neolib::segmented_array<unsigned char, chunk_size>::segment&
> >>> segment =
> >>> * * * * * * * * *segmented_iterator<unsigned char,
> >>> chunk_size>(buf_.begin()).segment();

>
> >>> * * * numWritten = Write(sock_,&buf_[0], segment.size());

>
> >> Undeclared function again.

>
> >>> * * * if (numWritten< * *segment.size()) goto ending;

>
> >> Ug.

>
> >>> * * * partialFlush_ = false;

>
> >> Lots of undefined stuff there, so can't comment.

>
> > Sorry. *Some are here --
> >http://webEbenezer.net/misc/SendBuffer.hh.
> > The Write function is here
> >http://webEbenezer.net/misc/IO.cc.

>
> But that doesn't excuse the gratuitous use of goto!
>


I started by asking for ideas on how to improve it.
Perhaps someone would show what they believe to be
an improvement. I realized after thinking about
your earlier comments that the member variable
partialFlush could be a static local variable.


bool
SendBuffer::Flush ()
{
uint32_t const all = buf_.size();
uint32_t numWritten = 0;
static bool partialFlush = false;
if (partialFlush) {
::neolib::segmented_array<unsigned char, chunk_size>::segment&
segment =
segmented_iterator<unsigned char,
chunk_size>(buf_.begin()).segment();

numWritten = Write(sock_, &buf_[0], segment.size());
if (numWritten < segment.size()) goto ending;
partialFlush = false;
}

{
int32_t const chunks = (all - numWritten) / chunk_size;
for (int32_t ii = 0; ii < chunks; ++ii) {
uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
numWritten += bytes;
if (bytes < chunk_size) goto ending;
}
}

if (numWritten < all) {
numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
}

ending:
buf_.erase(buf_.begin(), buf_.begin() + numWritten);
if (numWritten == all) return true;

partialFlush = true;
return false;
}


 
Reply With Quote
 
Fulvio Esposito
Guest
Posts: n/a
 
      09-16-2011
Hi Brian,
I'm not really an expert, but here a couple of suggestion

> bool
> SendBuffer::Flush ()
> {
> * uint32_t const all = buf_.size();
> * uint32_t numWritten = 0;


use std::size_t instead of uint32_t, it's more portable!

> * if (partialFlush_) {
> * * ::neolib::segmented_array<unsigned char, chunk_size>::segment&
> segment =
> * * * * * * * *segmented_iterator<unsigned char,
> chunk_size>(buf_.begin()).segment();
>
> * * numWritten = Write(sock_, &buf_[0], segment.size());
> * * if (numWritten < segment.size()) goto ending;
> * * partialFlush_ = false;
> * }


is partialFlush_ a flag about last flush operation and not the
current? That is, if a flush doesn't complete for any reason, you set
partial flush to true, and the next time flush is called, you send the
pending bytes (in fact, the entire segment that contains the first
byte in the buffer), am I right?
Maybe I can't see how useful this is 'cause I have not read all the
code, but if flush simply try to send the entire buffer, and if it
cannot, simply erase only the sended part and leave the rest to the
next call?

>
> * {
> * * int32_t const chunks = (all - numWritten) / chunk_size;
> * * for (int32_t ii = 0; ii < chunks; ++ii) {
> * * * uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
> * * * numWritten += bytes;
> * * * if (bytes < chunk_size) goto ending;
> * * }
> * }


Maybe the outer enclosing clock could be eliminated, it's not useful
here.



>
> * if (numWritten < all) {
> * * numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
> * }
>
> ending:
> * buf_.erase(buf_.begin(), buf_.begin() + numWritten);
> * if (numWritten == all) return true;
>
> * partialFlush_ = true;
> * return false;
>
> }


goto is considered worst practice maybe a redesign to eliminate it
could be better.

Bye,
Fulvio Esposito
 
Reply With Quote
 
Ebenezer
Guest
Posts: n/a
 
      09-16-2011
On Sep 16, 4:19*am, Fulvio Esposito <(E-Mail Removed)> wrote:
> Hi Brian,
> I'm not really an expert, but here a couple of suggestion
>
> > bool
> > SendBuffer::Flush ()
> > {
> > * uint32_t const all = buf_.size();
> > * uint32_t numWritten = 0;

>
> use std::size_t instead of uint32_t, it's more portable!
>
> > * if (partialFlush_) {
> > * * ::neolib::segmented_array<unsigned char, chunk_size>::segment&
> > segment =
> > * * * * * * * *segmented_iterator<unsigned char,
> > chunk_size>(buf_.begin()).segment();

>
> > * * numWritten = Write(sock_, &buf_[0], segment.size());
> > * * if (numWritten < segment.size()) goto ending;
> > * * partialFlush_ = false;
> > * }

>
> is partialFlush_ a flag about last flush operation and not the
> current?


Yes, it is about the previous flush call.

> That is, if a flush doesn't complete for any reason, you set
> partial flush to true, and the next time flush is called, you send the
> pending bytes (in fact, the entire segment that contains the first
> byte in the buffer), am I right?
> Maybe I can't see how useful this is 'cause I have not read all the
> code, but if flush simply try to send the entire buffer, and if it
> cannot, simply erase only the sended part and leave the rest to the
> next call?
>


The partialFlush stuff is there to detect if there's a
need to do some extra work on the first segment. This
function is used with a marshalling library... we only
add to the end of the container. So the only way the
first segment isn't full is if it also the last segment.



>
>
> > * {
> > * * int32_t const chunks = (all - numWritten) / chunk_size;
> > * * for (int32_t ii = 0; ii < chunks; ++ii) {
> > * * * uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
> > * * * numWritten += bytes;
> > * * * if (bytes < chunk_size) goto ending;
> > * * }
> > * }

>
> Maybe the outer enclosing clock could be eliminated, it's not useful
> here.
>
>
>
> > * if (numWritten < all) {
> > * * numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
> > * }

>
> > ending:
> > * buf_.erase(buf_.begin(), buf_.begin() + numWritten);
> > * if (numWritten == all) return true;

>
> > * partialFlush_ = true;
> > * return false;

>
> > }

>
> goto is considered worst practice maybe a redesign to eliminate it
> could be better.
>


Thanks for your comments. I'm not sure about a redesign.
That's why I asked.
 
Reply With Quote
 
Fulvio Esposito
Guest
Posts: n/a
 
      09-16-2011
> Thanks for your comments. I'm not sure about a redesign.
> That's why I asked.


mmm maybe I'm missing something, but why you are treating as error a write that send less bytes than what you asked? Why don't you use your PersistentWrite to make sure to send the entire buffer? In general a flush on a stream has the responsibility to empty the buffer sending the entire content to the socket, so its postcondition should be:

assert( buf_.empty() == true );

Why you allow partial flushing, exposing logic to clients of SendBuffer class? For example, the sync() method on standard streambuf have this semantic..

Obviously I don't know much about the context, so partial flush could be a desired behaviour.

Hope this help!

Cheers,
Fulvio
 
Reply With Quote
 
Ebenezer
Guest
Posts: n/a
 
      09-18-2011
On Sep 15, 7:23*pm, Ebenezer <(E-Mail Removed)> wrote:
> On Sep 15, 2:38*pm, Ian Collins <(E-Mail Removed)> wrote:
>
>
>
>
>
>
>
>
>
> > On 09/16/11 02:34 AM, Ebenezer wrote:

>
> > > On Sep 15, 12:08 am, Ian Collins<(E-Mail Removed)> *wrote:
> > >> On 09/15/11 05:01 PM, Ebenezer wrote:

>
> > >>> Shalom

>
> > >>> How to improve this function?

>
> > >>> bool
> > >>> SendBuffer::Flush ()
> > >>> {
> > >>> * * uint32_t const all = buf_.size();
> > >>> * * uint32_t numWritten = 0;

>
> > >> You're probably better off using size_t unless you have a really really
> > >> good reason to use uint32_t.

>
> > >>> * * if (partialFlush_) {
> > >>> * * * ::neolib::segmented_array<unsigned char, chunk_size>::segment&
> > >>> segment =
> > >>> * * * * * * * * *segmented_iterator<unsigned char,
> > >>> chunk_size>(buf_.begin()).segment();

>
> > >>> * * * numWritten = Write(sock_,&buf_[0], segment.size());

>
> > >> Undeclared function again.

>
> > >>> * * * if (numWritten< * *segment.size()) goto ending;

>
> > >> Ug.

>
> > >>> * * * partialFlush_ = false;

>
> > >> Lots of undefined stuff there, so can't comment.

>
> > > Sorry. *Some are here --
> > >http://webEbenezer.net/misc/SendBuffer.hh.
> > > The Write function is here
> > >http://webEbenezer.net/misc/IO.cc.

>
> > But that doesn't excuse the gratuitous use of goto!

>
> I started by asking for ideas on how to improve it.
> Perhaps someone would show what they believe to be
> an improvement. *I realized after thinking about
> your earlier comments that the member variable
> partialFlush could be a static local variable.
>
> bool
> SendBuffer::Flush ()
> {
> * uint32_t const all = buf_.size();
> * uint32_t numWritten = 0;
> * static bool partialFlush = false;
> * if (partialFlush) {
> * * ::neolib::segmented_array<unsigned char, chunk_size>::segment&
> segment =
> * * * * * * * *segmented_iterator<unsigned char,
> chunk_size>(buf_.begin()).segment();
>
> * * numWritten = Write(sock_, &buf_[0], segment.size());
> * * if (numWritten < segment.size()) goto ending;
> * * partialFlush = false;
> * }
>
> * {
> * * int32_t const chunks = (all - numWritten) / chunk_size;
> * * for (int32_t ii = 0; ii < chunks; ++ii) {
> * * * uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
> * * * numWritten += bytes;
> * * * if (bytes < chunk_size) goto ending;
> * * }
> * }
>
> * if (numWritten < all) {
> * * numWritten += Write(sock_, &buf_[numWritten], all - numWritten);
> * }
>
> ending:
> * buf_.erase(buf_.begin(), buf_.begin() + numWritten);
> * if (numWritten == all) return true;
>
> * partialFlush = true;
> * return false;
>
>
>
>
>
>
>
> }



How would you rate this function -- with a 1 being terrible
and a 10 being excellent?
 
Reply With Quote
 
Ian Collins
Guest
Posts: n/a
 
      09-18-2011
On 09/19/11 08:30 AM, Ebenezer wrote:
> On Sep 15, 7:23 pm, Ebenezer<(E-Mail Removed)> wrote:
>>
>> I started by asking for ideas on how to improve it.
>> Perhaps someone would show what they believe to be
>> an improvement. I realized after thinking about
>> your earlier comments that the member variable
>> partialFlush could be a static local variable.
>>
>> bool
>> SendBuffer::Flush ()
>> {
>> uint32_t const all = buf_.size();
>> uint32_t numWritten = 0;
>> static bool partialFlush = false;
>> if (partialFlush) {
>> ::neolib::segmented_array<unsigned char, chunk_size>::segment&
>> segment =
>> segmented_iterator<unsigned char,
>> chunk_size>(buf_.begin()).segment();
>>
>> numWritten = Write(sock_,&buf_[0], segment.size());
>> if (numWritten< segment.size()) goto ending;
>> partialFlush = false;
>> }
>>
>> {
>> int32_t const chunks = (all - numWritten) / chunk_size;
>> for (int32_t ii = 0; ii< chunks; ++ii) {
>> uint32_t bytes = Write(sock_,&buf_[numWritten], chunk_size);
>> numWritten += bytes;
>> if (bytes< chunk_size) goto ending;
>> }
>> }
>>
>> if (numWritten< all) {
>> numWritten += Write(sock_,&buf_[numWritten], all - numWritten);
>> }
>>
>> ending:
>> buf_.erase(buf_.begin(), buf_.begin() + numWritten);
>> if (numWritten == all) return true;
>>
>> partialFlush = true;
>> return false;
>> }

>
>
> How would you rate this function -- with a 1 being terrible
> and a 10 being excellent?


You don't appear to have headed any of the suggestions.

At review, I'd still give it a 1 because it isn't clear what it does,
why it uses fixed width types and it still contains gratuitous gotos.

--
Ian Collins
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
thoughts on a more generic Array#partition function Rudi Cilibrasi Ruby 13 07-04-2008 03:31 PM
New Hash Function workshop thoughts ShadowEyez Computer Security 0 11-04-2005 04:59 AM
Please share your thoughts on Member Function Templates Rajan C++ 4 05-27-2005 04:09 PM
write a function such that when ever i call this function in some other function .it should give me tha data type and value of calling function parameter komal C++ 6 01-25-2005 11:13 AM
Passing a C++ object's member function to a C function expecing a function pointer! James Vanns C++ 7 01-21-2004 02:39 AM



Advertisments