Jump to content

  • Log In with Google      Sign In   
  • Create Account

We're offering banner ads on our site from just $5!

1. Details HERE. 2. GDNet+ Subscriptions HERE. 3. Ad upload HERE.


Goto here?


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
15 replies to this topic

#1 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 13 September 2011 - 08:28 PM

Currently I have this:

bool
SendBuffer::Flush ()
{
  uint32_t all = buf_.size();
  bool fullTilt = true;
  ::neolib::segmented_array<unsigned char, chunk_size>::segment& segment = 
 			segmented_iterator<unsigned char, chunk_size>(buf_.begin()).segment();

  uint32_t numWritten = Write(sock_, &buf_[0], segment.size());
  if (numWritten < segment.size()) fullTilt = false;

  while (all - numWritten > chunk_size && fullTilt) {
	uint32_t bytes = Write(sock_, &buf_[numWritten], chunk_size);
	numWritten += bytes;
	if (bytes < chunk_size) fullTilt = false;
  }

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

  buf_.erase(buf_.begin(), buf_.begin() + numWritten);
  return numWritten == all;
}

But am tempted to do this:

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

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

  while (all - numWritten > chunk_size) {
	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);
  return numWritten == all;
}

Is that so bad?

Brian Wood
Ebenezer Enterprises
http://webEbenezer.net

Sponsor:

#2 smasherprog   Members   -  Reputation: 432

Like
0Likes
Like

Posted 13 September 2011 - 08:58 PM

when flushing the contents of a buffer that has a fixed chunksize that is sent, I use this
unsigned int chunkstosend = totalbytestosend/ chunk_size;// this will give you the number of chunks you will be sending
for(unsigned int i=0; i<chunkstosend; i++){
	Write(sock_, &buf_[numWritten], chunk_size);
}
unsigned int remainder =totalbytestosend -  (chunkstosend* chunk_size);// this is the remaining bytes to send if there are any
Write(sock_, &buf_[numWritten], remainder);// Write should check for a size of 0 before sending. If it doesnt, then check for remainder == zero before calling Write in an if statement
}

The above code works because integer division rounds down in c++
Wisdom is knowing when to shut up, so try it.
--Game Development http://nolimitsdesigns.com: Reliable UDP library, Threading library, Math Library, UI Library. Take a look, its all free.

#3 latent   Members   -  Reputation: 139

Like
0Likes
Like

Posted 13 September 2011 - 09:24 PM

When I encounter goto or a flag variable and am in an if-it-ain't-broke-fix-it-anyway mood, I'll often change the code to eliminate it, simply because I find it easier to read, and I hate maintaining unnecessarily convoluted code. Clearly the pedant in me has no care for whether or not it's the most perfectly efficient solution:



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

	uint32_t numWritten = Write(sock_, &buf_[0], segment.size());
	if (!(numWritten < segment.size())) {
		while (numWritten < all) {
			uint32_t bytes = Write(sock_, &buf_[numWritten], min(chunk_size, all-numWritten));
			numWritten += bytes;
			if (bytes != chunk_size)
				break;
		}
	}
	
	buf_.erase(buf_.begin(), buf_.begin() + numWritten);
	return numWritten == all;
}


#4 ApochPiQ   Moderators   -  Reputation: 16079

Like
1Likes
Like

Posted 13 September 2011 - 09:53 PM

I read your code five times before I found the goto.

Given that it hurts readability that much, I'd say stick with your current solution. There is no gain to adding non-obvious control flow in this situation.

#5 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 14 September 2011 - 07:59 AM

when flushing the contents of a buffer that has a fixed chunksize that is sent, I use this



Thanks for the replies. There's a possibility of incomplete flushes. In that case the first segment may have less than chunk_size elements on subsequent calls to Flush.

#6 Álvaro   Crossbones+   -  Reputation: 13662

Like
0Likes
Like

Posted 14 September 2011 - 08:45 AM

I found the code with goto in it easy to read, and I prefer it to your code with a flag. I would use capital letters for the label so it sticks out more. Perhaps you can give it a name that documents why you are doing it, like BREAK_SKIPPING_LAST_WRITE.

#7 VReality   Members   -  Reputation: 436

Like
1Likes
Like

Posted 14 September 2011 - 07:34 PM

How about this:
bool SendBuffer::Flush()
{
  uint32_t all = buf_.size();
  ::neolib::segmented_array<unsigned char, chunk_size>::segment& segment = segmented_iterator<unsigned char, chunk_size>(buf_.begin()).segment();

  uint32_t totalWritten = Write(sock_, &buf_[0], segment.size());

  uint32_t wroteThisTime = chunk_size;
  while((totalWritten < all) && (wroteThisTime == chunk_size))
	totalWritten += wroteThisTime = Write(sock_, &buf_[totalWritten], std::min(all - totalWritten, chunk_size));

  buf_.erase(buf_.begin(), buf_.begin() + totalWritten);
  return totalWritten == all;
}
EDIT: To new readers - This code has a bug. Read on...

#8 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 16 September 2011 - 10:13 PM

How about this:

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

  uint32_t totalWritten = Write(sock_, &buf_[0], segment.size());

  uint32_t wroteThisTime = chunk_size;
  while((totalWritten < all) && (wroteThisTime == chunk_size))
    totalWritten += wroteThisTime = Write(sock_, &buf_[totalWritten], std::min(all - totalWritten, chunk_size));

  buf_.erase(buf_.begin(), buf_.begin() + totalWritten);
  return totalWritten == all;
}


I think that has problems. If the first call to Write returns less than the segment size, the second call to Write won't be aligned properly. This could lead to incorrect data being sent. I'm not keen on the std::min use there.

#9 iMalc   Crossbones+   -  Reputation: 2313

Like
0Likes
Like

Posted 17 September 2011 - 03:43 PM

I think that has problems. If the first call to Write returns less than the segment size, the second call to Write won't be aligned properly. This could lead to incorrect data being sent. I'm not keen on the std::min use there.

No, that's not true. The " && (wroteThisTime == chunk_size)" part in the while loop will ensure that it exits the loop it the number of bytes written is not the amount intended.
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

#10 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 17 September 2011 - 06:52 PM

[No, that's not true. The " && (wroteThisTime == chunk_size)" part in the while loop will ensure that it exits the loop it the number of bytes written is not the amount intended.


But it goes into the while loop at least once ...
uint32_t wroteThisTime = chunk_size;
The Write in the loop may use chunk_size as the number of bytes to write. If totalWritten isn't the index of the first element in a segment, it will send data that's past the end of the segment.

#11 iMalc   Crossbones+   -  Reputation: 2313

Like
0Likes
Like

Posted 18 September 2011 - 01:10 PM

Okay, you got me. I was thinking of between iterations of the loop
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

#12 VReality   Members   -  Reputation: 436

Like
0Likes
Like

Posted 19 September 2011 - 10:07 PM

I think that has problems. If the first call to Write returns less than the segment size, the second call to Write won't be aligned properly. This could lead to incorrect data being sent...


Yep it's missing an "if". Although I think the real problem is that if the first call returns less than the segment size, and also less than "all", then it failed and we don't catch the failure. If it doesn't return less than "all", then the loop isn't run anyway, 'cause we're done writing .

But the point is that with a little bit of restructure it's every bit as readable as before (significantly, it's readable enough for the logic flaw to jump out at you), with simpler flow control, and no temptation to use a "goto".

In short, no need for "goto" here.

I'm not keen on the std::min use there.


What is it about that use that you don't like?

#13 Codarki   Members   -  Reputation: 462

Like
-1Likes
Like

Posted 20 September 2011 - 04:53 AM

But the point is that with a little bit of restructure it's every bit as readable as before (significantly, it's readable enough for the logic flaw to jump out at you), with simpler flow control, and no temptation to use a "goto".

In short, no need for "goto" here.

It might be more readable, but that doesn't matter if it's wrong. I think latent already posted working solution at post #3.

#14 wodinoneeye   Members   -  Reputation: 861

Like
0Likes
Like

Posted 21 September 2011 - 05:49 AM

I read your code five times before I found the goto.

Given that it hurts readability that much, I'd say stick with your current solution. There is no gain to adding non-obvious control flow in this situation.


Funny I saw it instantly. But then Im used to using gotos all the time.

It usually helps to have the label name all it CAPS to make it stand out more (meaningfully worded labels are hard to misunderstand)

I never seem to consider a single goto near a procedures exit as 'convoluted' (again maybe Ive seen ALOT more really convoluted code including
alot where NOT using goto s causes very deep ugly indentations of layers of escape logic instead.
--------------------------------------------Ratings are Opinion, not Fact

#15 VReality   Members   -  Reputation: 436

Like
0Likes
Like

Posted 23 September 2011 - 04:44 AM

It might be more readable...


Although I certainly never claimed it was. Readability is at least partially dependent on the reader, and my code was in a different style than the OP's.


...but that doesn't matter if it's wrong.


Of course it does.

Easily readable code which is broken is readily fixed, the error in my code being case-in-point. Working code which is unreadable, on the other hand, will eventually break and prove most resistant to maintenance. Readability matters - in a sense, almost more than functionality.

But anyway, neither the simplified flow control, nor the readability of the code, would be compromised by adding the needed "if". So the fact that the code is "wrong" is all but irrelevant to the improvements it's getting at - just like the "working solution" already posted.

#16 Codarki   Members   -  Reputation: 462

Like
0Likes
Like

Posted 23 September 2011 - 10:31 AM

Easily readable code which is broken is readily fixed, the error in my code being case-in-point. Working code which is unreadable, on the other hand, will eventually break and prove most resistant to maintenance. Readability matters - in a sense, almost more than functionality.

I agree with you that readability matters a lot. I actually sketched some SocketWriter class to reply in this, it would make this function pretty easy to digest (and the a lot of this SendBuffer class much more simpler). But I decided not to post, because it would be better for SendBuffer to own instance of SocketWriter and I don't know how the rest of the class looks like..




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS