OK, this is already a lengthy rebuttal, so if no one has the patience to keep this going, I understand -- feel free to let it die on the vine and I'll let it go. But since I do honestly want to understand the contrary POV...
Back to Doc:
Docedison:
I feel strongly that justifying the use of obfuscating code that easily generates more, similar code is just that or a clever method of emphasizing the true value of Goto... Why not the two examples presented.
Clarity (as the opposite of obfuscation) is one of the main reasons I use it when I do. De-duplication is the other. This, I think, is one of the opposite viewpoints I'm really struggling to understand.
If I may, here's the initialization part of the example code, once with goto, and then re-written without goto:
// This is timing critical
flags = SREG;
cli();
// Initialize hardware
rval = dev_init();
if (rval != ERR_OK) goto cleanup;
// Set hardware to whatever appropriate mode
rval = dev_set_mode(SOME_FLAG);
if (rval != ERR_OK) goto cleanup;
...
cleanup:
// We're done with the device now
dev_close();
// Restore previous state
SREG = flags;
return rval;
// This is timing critical
flags = SREG;
cli();
// Initialize hardware
rval = dev_init();
if (rval != ERR_OK) {
SREG = flags;
return rval;
}
// Set hardware to whatever appropriate mode
rval = dev_set_mode(SOME_FLAG);
if (rval != ERR_OK) {
dev_close();
SREG = flags;
return rval;
}
Regarding obfuscation: I think the intent and flow of the goto version is pretty darn clear. However, in this particular example, I'm at the threshold of whether I would prefer using goto with a single cleanup block, or to handle errors individually, as in the second example. Afterall, the first block doesn't need to close the device (it wasn't opened successfully), BUT, if the close() function is specifically tolerant of this condition (as in the case of free(NULL) for example), the justification (for me) would go something like this:
- With two or three exit points, it's probably best to just handle it locally and prevent separating the code.
- With more than three, it becomes likely I (or someone following me) will forget to address all the necessary cleanup steps each and every time. Using a single exit routine can prevent this bug from happening, and de-clutter the main body significantly.
- With more than three, the reduction in duplicated code could be a worthy goal in and of itself -- at least in an embedded environment with potentially limited flash. (I make this a point in my own libraries, which may run on anything from a Tiny13 to a Mega2560.)
- Any time there's a long or particularly tricky cleanup block, I'll lean toward separating it.
So, let's try removing some of the other gotos:
Before:
while ( ((rval = dev_query()) != ERR_OK) && (data_len != 0) ) {
// Check for timeout
if ( (rval == ERR_BUSY) && ( (millis() - now) < timeout) ) {
goto cleanup;
}
// Check for fatal error
if (rval == ERR_FATAL) {
goto cleanup;
}
After:
while ( ((rval = dev_query()) != ERR_OK) && (data_len != 0) ) {
// Check for timeout
if ( (rval == ERR_BUSY) && ( (millis() - now) < timeout) ) break;
// Check for fatal error
if (rval == ERR_FATAL) break;
I have no qualms with the goto-free version of this. The original example is slightly flawed in this regard, as it isn't complex enough to really warrant any particular method. Any of a handful of options all get the job done with equal effectiveness. If this code were verbatim production code, I might still lean toward goto because it fits well enough before the loop, within the outer loop, and within the inner loop. Given there's no other penalty for doing so, consistency becomes the motivator, and I'll choose one approach and stick to it for the sake of making it easier to spot all the exit points. (Again, to eschew obfuscation!) But I'll agree it isn't strictly necessary here.
Moving on to the inner loop, the best way I can see to refactor this requires changes to the outer loop as well. Here we go:
Before:
while ( ((rval = dev_query()) != ERR_OK) && (data_len != 0) ) {
// Check for timeout
if ( (rval == ERR_BUSY) && ( (millis() - now) < timeout) ) {
goto cleanup;
}
// Check for fatal error
if (rval == ERR_FATAL) {
goto cleanup;
}
// Send data to device
while (data_len) {
rval = dev_send8(data);
// Device will signal when its buffer is full
if (rval == ERR_BUFFER_FULL) {
// Fall back to wait loop
break;
}
// Handle other errors
if (rval != ERR_OK) {
goto cleanup;
}
// Update data pointer and size
data++;
data_len--;
} // inner loop
} // outer loop
After:
while (data_len) {
// Get device status update
rval = dev_query();
// Check for timeout
if ( (rval == ERR_BUSY) && ( (millis() - now) < timeout) )
break;
// Check for fatal error
if (rval == ERR_FATAL) break;
// Send data to device
while (data_len && (rval != ERR_OK) ) {
rval = dev_send8(data);
// Check for send failures
if (rval != ERR_OK) break;
// Update data pointer and size
data++;
data_len--;
} // inner loop
// Wait for device to service a full buffer
// but bail out on fatal errors
if ( (rval != ERR_OK) && (rval != ERR_BUFFER_FULL) ) break;
} // outer loop
Honestly, with this particular code, the goto-less version of this might indeed be better. Maybe I'm doing more to disprove my stance here, and that's fine... with a trivial example, there are lots of acceptable permutations. I'm not trying to be a champion for the use of goto specifically, but I do think it's worth a fresh perspective. I don't like that people bear so much hatred for something that does its job as intended, and is often (IMO!) no better or worse than some alternative. Taken on its own merits, it seems perfectly reasonable to use it.
Thoughts? Or have we reached a point where we just have to agree to disagree? (If so, that's fine. I just don't want to be stubborn about it if there's something I'm truly missing.)