Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c++ > #10121 > unrolled thread

Thoughts on function

Back to article view | Back to comp.lang.c++

Contents

  Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-14 22:01 -0700
    Re: Thoughts on function Ian Collins <ian-news@hotmail.com> - 2011-09-15 17:08 +1200
      Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-15 07:34 -0700
        Re: Thoughts on function Ian Collins <ian-news@hotmail.com> - 2011-09-16 07:38 +1200
          Re: Thoughts on function Leigh Johnston <leigh@i42.co.uk> - 2011-09-15 20:40 +0100
          Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-15 17:23 -0700
            Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-18 13:30 -0700
              Re: Thoughts on function Ian Collins <ian-news@hotmail.com> - 2011-09-19 09:34 +1200
                Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-18 18:25 -0700
                  Re: Thoughts on function Geoff <geoff@invalid.invalid> - 2011-09-18 19:08 -0700
                    Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-18 19:46 -0700
                      Re: Thoughts on function Geoff <geoff@invalid.invalid> - 2011-09-18 20:12 -0700
                        Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-18 21:28 -0700
                    Re: Thoughts on function Miles Bader <miles@gnu.org> - 2011-09-19 13:26 +0900
                      Re: Thoughts on function Ian Collins <ian-news@hotmail.com> - 2011-09-19 16:45 +1200
                        Re: Thoughts on function Miles Bader <miles@gnu.org> - 2011-09-19 15:19 +0900
                  Re: Thoughts on function Ian Collins <ian-news@hotmail.com> - 2011-09-19 16:43 +1200
                    Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-19 11:16 -0700
                      Re: Thoughts on function Ian Collins <ian-news@hotmail.com> - 2011-09-20 07:44 +1200
                        Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-19 18:03 -0700
                          Re: Thoughts on function Ian Collins <ian-news@hotmail.com> - 2011-09-20 13:15 +1200
                            Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-19 18:27 -0700
    Re: Thoughts on function Fulvio Esposito <esposito.fulvio@gmail.com> - 2011-09-16 02:19 -0700
      Re: Thoughts on function Ebenezer <woodbrian77@gmail.com> - 2011-09-16 11:20 -0700
        Re: Thoughts on function Fulvio Esposito <esposito.fulvio@gmail.com> - 2011-09-16 13:51 -0700
    Re: Thoughts on function hanukas <jukka@liimatta.org> - 2011-09-19 06:19 -0700

Page 1 of 2  [1] 2  Next page →

#10121 — Thoughts on function

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

[toc] | [next] | [standalone]

#10122

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

[toc] | [prev] | [next] | [standalone]

#10174

On Sep 15, 12:08 am, Ian Collins <ian-n...@hotmail.com> 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 .

[toc] | [prev] | [next] | [standalone]

#10179

On 09/16/11 02:34 AM, Ebenezer wrote:
> On Sep 15, 12:08 am, Ian Collins<ian-n...@hotmail.com>  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

[toc] | [prev] | [next] | [standalone]

#10180

On 15/09/2011 20:38, Ian Collins wrote:
> On 09/16/11 02:34 AM, Ebenezer wrote:
>> On Sep 15, 12:08 am, Ian Collins<ian-n...@hotmail.com> 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!

Does this now mean that yet another long and tedious thread about goto 
will start here?

/Leigh

[toc] | [prev] | [next] | [standalone]

#10188

On Sep 15, 2:38 pm, Ian Collins <ian-n...@hotmail.com> wrote:
> On 09/16/11 02:34 AM, Ebenezer wrote:
>
>
>
>
>
>
>
>
>
> > On Sep 15, 12:08 am, Ian Collins<ian-n...@hotmail.com>  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;
}

[toc] | [prev] | [next] | [standalone]

#10257

On Sep 15, 7:23 pm, Ebenezer <woodbria...@gmail.com> wrote:
> On Sep 15, 2:38 pm, Ian Collins <ian-n...@hotmail.com> wrote:
>
>
>
>
>
>
>
>
>
> > On 09/16/11 02:34 AM, Ebenezer wrote:
>
> > > On Sep 15, 12:08 am, Ian Collins<ian-n...@hotmail.com>  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?

[toc] | [prev] | [next] | [standalone]

#10258

On 09/19/11 08:30 AM, Ebenezer wrote:
> On Sep 15, 7:23 pm, Ebenezer<woodbria...@gmail.com>  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

[toc] | [prev] | [next] | [standalone]

#10267

On Sep 18, 4:34 pm, Ian Collins <ian-n...@hotmail.com> wrote:
> On 09/19/11 08:30 AM, Ebenezer wrote:
>
>
>
>
>
>
>
>
>
> > On Sep 15, 7:23 pm, Ebenezer<woodbria...@gmail.com>  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.

I'm not opposed to the size_t suggestion, but am wanting to
focus on the bigger picture.

>
> 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.
>

All of the code is free -- gratuitous.  Meanwhile no one has
provided an alternative they think is better.

[toc] | [prev] | [next] | [standalone]

#10268

On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer
<woodbrian77@gmail.com> wrote:

>All of the code is free -- gratuitous.  Meanwhile no one has
>provided an alternative they think is better.

Gratuitous in this context means unnecessary. Not gratis. But you knew
that.

Refactor your function, get rid of the goto's. They are bad practice. 

Don't worry about writing redundant lines in your functions, don't try
to optimize them away with goto's as you have done in your function,
let the compiler optimize them away, it's much better at it than you
are. What you should be writing for is clarity and maintainability.

[toc] | [prev] | [next] | [standalone]

#10269

On Sep 18, 9:08 pm, Geoff <ge...@invalid.invalid> wrote:
> On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer
>
> <woodbria...@gmail.com> wrote:
> >All of the code is free -- gratuitous.  Meanwhile no one has
> >provided an alternative they think is better.
>
> Gratuitous in this context means unnecessary. Not gratis. But you knew
> that.
>
> Refactor your function, get rid of the goto's. They are bad practice.
>
> Don't worry about writing redundant lines in your functions, don't try
> to optimize them away with goto's as you have done in your function,
> let the compiler optimize them away, it's much better at it than you
> are. What you should be writing for is clarity and maintainability.

Still no code.

[toc] | [prev] | [next] | [standalone]

#10270

On Sun, 18 Sep 2011 19:46:06 -0700 (PDT), Ebenezer
<woodbrian77@gmail.com> wrote:

>On Sep 18, 9:08 pm, Geoff <ge...@invalid.invalid> wrote:
>> On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer
>>
>> <woodbria...@gmail.com> wrote:
>> >All of the code is free -- gratuitous.  Meanwhile no one has
>> >provided an alternative they think is better.
>>
>> Gratuitous in this context means unnecessary. Not gratis. But you knew
>> that.
>>
>> Refactor your function, get rid of the goto's. They are bad practice.
>>
>> Don't worry about writing redundant lines in your functions, don't try
>> to optimize them away with goto's as you have done in your function,
>> let the compiler optimize them away, it's much better at it than you
>> are. What you should be writing for is clarity and maintainability.
>
>Still no code.

Ah! So it's not about what do you think of this function but it's
about fix this function if you can. We gave you our thoughts on this
function. We are not obligated to fix it for you. 

[toc] | [prev] | [next] | [standalone]

#10272

On Sep 18, 10:12 pm, Geoff <ge...@invalid.invalid> wrote:
> On Sun, 18 Sep 2011 19:46:06 -0700 (PDT), Ebenezer
>
>
>
>
>
> <woodbria...@gmail.com> wrote:
> >On Sep 18, 9:08 pm, Geoff <ge...@invalid.invalid> wrote:
> >> On Sun, 18 Sep 2011 18:25:40 -0700 (PDT), Ebenezer
>
> >> <woodbria...@gmail.com> wrote:
> >> >All of the code is free -- gratuitous. Meanwhile no one has
> >> >provided an alternative they think is better.
>
> >> Gratuitous in this context means unnecessary. Not gratis. But you knew
> >> that.
>
> >> Refactor your function, get rid of the goto's. They are bad practice.
>
> >> Don't worry about writing redundant lines in your functions, don't try
> >> to optimize them away with goto's as you have done in your function,
> >> let the compiler optimize them away, it's much better at it than you
> >> are. What you should be writing for is clarity and maintainability.
>
> >Still no code.
>
> Ah! So it's not about what do you think of this function but it's
> about fix this function if you can. We gave you our thoughts on this
> function. We are not obligated to fix it for you.

I have said numerous times I'm looking for an alternative
that is thought to be better.

[toc] | [prev] | [next] | [standalone]

#10271

Geoff <geoff@invalid.invalid> writes:
> Refactor your function, get rid of the goto's. They are bad practice. 

But let's be fair -- while goto is often[*] used badly, and in such
cases makes code harder to understand/maintain, it isn't somehow the
kiss of death.  There are reasonable uses of goto.

So although the presence of "goto" in code is a big warning sign
(especially in C++, where RIAA tends to supplant "goto for error
cleanup" idioms), is seems unreasonable to thus grade code a "1"
without actually looking at the details of how its used.

[*] maybe less so these days than in the past, as it's acquired such a
huge negative reputation that non-clueful programmers are likely to
avoid it entirely on that basis.

-Miles

-- 
Learning, n. The kind of ignorance distinguishing the studious.

[toc] | [prev] | [next] | [standalone]

#10274

On 09/19/11 04:26 PM, Miles Bader wrote:
> Geoff<geoff@invalid.invalid>  writes:
>> Refactor your function, get rid of the goto's. They are bad practice.
>
> But let's be fair -- while goto is often[*] used badly, and in such
> cases makes code harder to understand/maintain, it isn't somehow the
> kiss of death.  There are reasonable uses of goto.
>
> So although the presence of "goto" in code is a big warning sign
> (especially in C++, where RIAA tends to supplant "goto for error
> cleanup" idioms), is seems unreasonable to thus grade code a "1"
> without actually looking at the details of how its used.

Every coding standard I've written or had to follow has prohibited goto 
except in exceptional (and well commented) cases.

-- 
Ian Collins

[toc] | [prev] | [next] | [standalone]

#10275

Ian Collins <ian-news@hotmail.com> writes:
>> But let's be fair -- while goto is often[*] used badly, and in such
>> cases makes code harder to understand/maintain, it isn't somehow the
>> kiss of death.  There are reasonable uses of goto.
>>
>> So although the presence of "goto" in code is a big warning sign
>> (especially in C++, where RIAA tends to supplant "goto for error
>> cleanup" idioms), is seems unreasonable to thus grade code a "1"
>> without actually looking at the details of how its used.
>
> Every coding standard I've written or had to follow has prohibited
> goto except in exceptional (and well commented) cases.

That isn't inconsistent with what I said.

It isn't unreasonable for a coding standard, especially those targeted
at non-expert programmers, to outlaw goto with documented exceptions
like that -- the entire point of a coding standard, after all, is to
try and model subjective judgement with something simpler, more
consistent (within the organization), and easier to apply; this is
very valuable when many programmers of varying skill-levels are
working together.

But they aren't an end in themselves, and are not, in the end,
some sort of authoritative statement.

So if the OP was saying "goto gets your code grade 1 _in our
organization, because it violates our coding standards_", that would
be reasonable.  But of course that isn't the scenario here.

[Remember, I'm _not_ defending the OP's use of goto, I'm simply saying
that goto isn't _universally_ bad, and a nuanced discussion of code
quality shouldn't use simplistic shortcuts like that, even if they're
justified in many more practical applications.]

-Miles

-- 
On a bad day, I see brewers still talking as though all beer were
consumed in the pub, by the pint -- by insatiably thirsty Yorkshire
steelworkers who have in reality long ago sought work as striptease
artists.  [Michael Jackson]

[toc] | [prev] | [next] | [standalone]

#10273

On 09/19/11 01:25 PM, Ebenezer wrote:
> On Sep 18, 4:34 pm, Ian Collins<ian-n...@hotmail.com>  wrote:
>> On 09/19/11 08:30 AM, Ebenezer wrote:
>>
Why does google put in these huge gaps?  I didn't include it.
>>
>>> On Sep 15, 7:23 pm, Ebenezer<woodbria...@gmail.com>    wrote:
>>
>>>> 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.
>
> I'm not opposed to the size_t suggestion, but am wanting to
> focus on the bigger picture.
>
>> 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.
>
> All of the code is free -- gratuitous.  Meanwhile no one has
> provided an alternative they think is better.

Everything at after your ending label could be in a bool returning 
member function.

-- 
Ian Collins

[toc] | [prev] | [next] | [standalone]

#10297

On Sep 18, 11:43 pm, Ian Collins <ian-n...@hotmail.com> wrote:
> On 09/19/11 01:25 PM, Ebenezer wrote:> On Sep 18, 4:34 pm, Ian Collins<ian-n...@hotmail.com>  wrote:
> >> On 09/19/11 08:30 AM, Ebenezer wrote:
>
> Why does google put in these huge gaps?  I didn't include it.
>

Not sure.  Sorry.

>
>
>
> >>> On Sep 15, 7:23 pm, Ebenezer<woodbria...@gmail.com>    wrote:
>
> >>>> 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.
>
> > I'm not opposed to the size_t suggestion, but am wanting to
> > focus on the bigger picture.
>
> >> 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.
>
> > All of the code is free -- gratuitous.  Meanwhile no one has
> > provided an alternative they think is better.
>
> Everything at after your ending label could be in a bool returning
> member function.
>

OK, so if I work on an implementation like that would you
suggest making some of the local variables member variables?
The added function needs to access numWritten, all and partialFlush.

[toc] | [prev] | [next] | [standalone]

#10301

On 09/20/11 06:16 AM, Ebenezer wrote:
> On Sep 18, 11:43 pm, Ian Collins<ian-n...@hotmail.com>  wrote:
>> On 09/19/11 01:25 PM, Ebenezer wrote:>  On Sep 18, 4:34 pm, Ian Collins<ian-n...@hotmail.com>    wrote:
>>>> On 09/19/11 08:30 AM, Ebenezer wrote:
>>>>> On Sep 15, 7:23 pm, Ebenezer<woodbria...@gmail.com>      wrote:
>>
>>>>>> 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.
>>
>>> I'm not opposed to the size_t suggestion, but am wanting to
>>> focus on the bigger picture.
>>
>>>> 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.
>>
>>> All of the code is free -- gratuitous.  Meanwhile no one has
>>> provided an alternative they think is better.
>>
>> Everything at after your ending label could be in a bool returning
>> member function.
>>
>
> OK, so if I work on an implementation like that would you
> suggest making some of the local variables member variables?
> The added function needs to access numWritten, all and partialFlush.

Well you've already made partialFlush static, so it's a obvious 
contender for a a member variable.  The other two could be parameters.

-- 
Ian Collins

[toc] | [prev] | [next] | [standalone]

#10308

On Sep 19, 2:44 pm, Ian Collins <ian-n...@hotmail.com> wrote:
>
> Well you've already made partialFlush static, so it's a obvious
> contender for a a member variable.  The other two could be parameters.
>

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

  partialFlush_ = true;
  return false;
}


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()) return ending(all, numWritten);
    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) return ending(all, numWritten);
  }

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

  return ending(all, numWritten);
}


The .o file is 48 bytes bigger in this form when using
g++ 4.5.1 and -O3.  I'm not sure this is a step in the
right direction.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | comp.lang.c++

csiph-web