Clean ways to do multiple undos in C
up vote
12
down vote
favorite
Someone will probably say something about exceptions... but in C, what are other ways to do the following cleanly/clearly and without repeating so much code?
if (Do1()) { printf("Failed 1"); return 1; }
if (Do2()) { Undo1(); printf("Failed 2"); return 2; }
if (Do3()) { Undo2(); Undo1(); printf("Failed 3"); return 3; }
if (Do4()) { Undo3(); Undo2(); Undo1(); printf("Failed 4"); return 4; }
if (Do5()) { Undo4(); Undo3(); Undo2(); Undo1(); printf("Failed 5"); return 5; }
Etc...
This might be one case for using gotos. Or maybe multiple inner functions...
c
|
show 3 more comments
up vote
12
down vote
favorite
Someone will probably say something about exceptions... but in C, what are other ways to do the following cleanly/clearly and without repeating so much code?
if (Do1()) { printf("Failed 1"); return 1; }
if (Do2()) { Undo1(); printf("Failed 2"); return 2; }
if (Do3()) { Undo2(); Undo1(); printf("Failed 3"); return 3; }
if (Do4()) { Undo3(); Undo2(); Undo1(); printf("Failed 4"); return 4; }
if (Do5()) { Undo4(); Undo3(); Undo2(); Undo1(); printf("Failed 5"); return 5; }
Etc...
This might be one case for using gotos. Or maybe multiple inner functions...
c
1
Just to point out with regard to my previous comment: in these cases, it is generally the case that these cleanup actions need to be also performed when there are no early abortions (hence the mention of free/fclose); that makes the structure with goto & labels fairly straightforward and easy to read. This may not be the case that you are thinking of.
– 9769953
2 days ago
2
I believe you need to clarify the types of the functions, as we are getting all kinds of mixed answers. Are they the same type, or are all functions of different types?
– Lundin
2 days ago
1
@Lundin It is clear the code is an example of the code patterns that appear in many C functions, typically those that allocate resources. Precisely because the types do not matter here, they are not specified. It is pseudocode, if you want.
– Acorn
2 days ago
5
@Lundin No, I haven't said anything like it. And no, pseudo-code can be perfectly on topic.
– Acorn
2 days ago
3
@9769953 - I'd say the problem wasn'tgoto fail;
so much as avoiding curly braces. And it's not like it's a new sort of bug. The offending line didn't have to be agoto
, but could just as easily have been aexit(EXIT_FAILURE)
. Still the same bug, despite the different manifestation.
– StoryTeller
2 days ago
|
show 3 more comments
up vote
12
down vote
favorite
up vote
12
down vote
favorite
Someone will probably say something about exceptions... but in C, what are other ways to do the following cleanly/clearly and without repeating so much code?
if (Do1()) { printf("Failed 1"); return 1; }
if (Do2()) { Undo1(); printf("Failed 2"); return 2; }
if (Do3()) { Undo2(); Undo1(); printf("Failed 3"); return 3; }
if (Do4()) { Undo3(); Undo2(); Undo1(); printf("Failed 4"); return 4; }
if (Do5()) { Undo4(); Undo3(); Undo2(); Undo1(); printf("Failed 5"); return 5; }
Etc...
This might be one case for using gotos. Or maybe multiple inner functions...
c
Someone will probably say something about exceptions... but in C, what are other ways to do the following cleanly/clearly and without repeating so much code?
if (Do1()) { printf("Failed 1"); return 1; }
if (Do2()) { Undo1(); printf("Failed 2"); return 2; }
if (Do3()) { Undo2(); Undo1(); printf("Failed 3"); return 3; }
if (Do4()) { Undo3(); Undo2(); Undo1(); printf("Failed 4"); return 4; }
if (Do5()) { Undo4(); Undo3(); Undo2(); Undo1(); printf("Failed 5"); return 5; }
Etc...
This might be one case for using gotos. Or maybe multiple inner functions...
c
c
edited 2 days ago
Peter Mortensen
13.3k1983111
13.3k1983111
asked 2 days ago
dargaud
73511024
73511024
1
Just to point out with regard to my previous comment: in these cases, it is generally the case that these cleanup actions need to be also performed when there are no early abortions (hence the mention of free/fclose); that makes the structure with goto & labels fairly straightforward and easy to read. This may not be the case that you are thinking of.
– 9769953
2 days ago
2
I believe you need to clarify the types of the functions, as we are getting all kinds of mixed answers. Are they the same type, or are all functions of different types?
– Lundin
2 days ago
1
@Lundin It is clear the code is an example of the code patterns that appear in many C functions, typically those that allocate resources. Precisely because the types do not matter here, they are not specified. It is pseudocode, if you want.
– Acorn
2 days ago
5
@Lundin No, I haven't said anything like it. And no, pseudo-code can be perfectly on topic.
– Acorn
2 days ago
3
@9769953 - I'd say the problem wasn'tgoto fail;
so much as avoiding curly braces. And it's not like it's a new sort of bug. The offending line didn't have to be agoto
, but could just as easily have been aexit(EXIT_FAILURE)
. Still the same bug, despite the different manifestation.
– StoryTeller
2 days ago
|
show 3 more comments
1
Just to point out with regard to my previous comment: in these cases, it is generally the case that these cleanup actions need to be also performed when there are no early abortions (hence the mention of free/fclose); that makes the structure with goto & labels fairly straightforward and easy to read. This may not be the case that you are thinking of.
– 9769953
2 days ago
2
I believe you need to clarify the types of the functions, as we are getting all kinds of mixed answers. Are they the same type, or are all functions of different types?
– Lundin
2 days ago
1
@Lundin It is clear the code is an example of the code patterns that appear in many C functions, typically those that allocate resources. Precisely because the types do not matter here, they are not specified. It is pseudocode, if you want.
– Acorn
2 days ago
5
@Lundin No, I haven't said anything like it. And no, pseudo-code can be perfectly on topic.
– Acorn
2 days ago
3
@9769953 - I'd say the problem wasn'tgoto fail;
so much as avoiding curly braces. And it's not like it's a new sort of bug. The offending line didn't have to be agoto
, but could just as easily have been aexit(EXIT_FAILURE)
. Still the same bug, despite the different manifestation.
– StoryTeller
2 days ago
1
1
Just to point out with regard to my previous comment: in these cases, it is generally the case that these cleanup actions need to be also performed when there are no early abortions (hence the mention of free/fclose); that makes the structure with goto & labels fairly straightforward and easy to read. This may not be the case that you are thinking of.
– 9769953
2 days ago
Just to point out with regard to my previous comment: in these cases, it is generally the case that these cleanup actions need to be also performed when there are no early abortions (hence the mention of free/fclose); that makes the structure with goto & labels fairly straightforward and easy to read. This may not be the case that you are thinking of.
– 9769953
2 days ago
2
2
I believe you need to clarify the types of the functions, as we are getting all kinds of mixed answers. Are they the same type, or are all functions of different types?
– Lundin
2 days ago
I believe you need to clarify the types of the functions, as we are getting all kinds of mixed answers. Are they the same type, or are all functions of different types?
– Lundin
2 days ago
1
1
@Lundin It is clear the code is an example of the code patterns that appear in many C functions, typically those that allocate resources. Precisely because the types do not matter here, they are not specified. It is pseudocode, if you want.
– Acorn
2 days ago
@Lundin It is clear the code is an example of the code patterns that appear in many C functions, typically those that allocate resources. Precisely because the types do not matter here, they are not specified. It is pseudocode, if you want.
– Acorn
2 days ago
5
5
@Lundin No, I haven't said anything like it. And no, pseudo-code can be perfectly on topic.
– Acorn
2 days ago
@Lundin No, I haven't said anything like it. And no, pseudo-code can be perfectly on topic.
– Acorn
2 days ago
3
3
@9769953 - I'd say the problem wasn't
goto fail;
so much as avoiding curly braces. And it's not like it's a new sort of bug. The offending line didn't have to be a goto
, but could just as easily have been a exit(EXIT_FAILURE)
. Still the same bug, despite the different manifestation.– StoryTeller
2 days ago
@9769953 - I'd say the problem wasn't
goto fail;
so much as avoiding curly braces. And it's not like it's a new sort of bug. The offending line didn't have to be a goto
, but could just as easily have been a exit(EXIT_FAILURE)
. Still the same bug, despite the different manifestation.– StoryTeller
2 days ago
|
show 3 more comments
14 Answers
14
active
oldest
votes
up vote
26
down vote
Yes, it's quite common to use goto in such cases to avoid repeating yourself.
An example:
int hello() {
int result;
if (Do1()) { result = 1; goto err_one; }
if (Do2()) { result = 2; goto err_two; }
if (Do3()) { result = 3; goto err_three; }
if (Do4()) { result = 4; goto err_four; }
if (Do5()) { result = 5; goto err_five; }
// Assuming you'd like to return 0 on success.
return 0;
err_five:
Undo4();
err_four:
Undo3();
err_three:
Undo2();
err_two:
Undo1();
err_one:
printf("Failed %i", result);
return result;
}
As always you probably also want to keep your functions small and batch together the operations in a meaningful manner to avoid a large "undo-code".
1
Note:return 0
may or may not be needed, depending on what the function is supposed to do.
– Acorn
2 days ago
1
@Acorn Without, code wouldn't be equivalent to code presented in question, which undoes only on error...
– Aconcagua
2 days ago
3
@Lundin I agree. This answer assumes that the Do and Undo functions might have different signatures in reality which is probably more common in practice.
– likle
2 days ago
10
@Lundin There is no code repetition here. Please, don't misunderstand generic examples with concrete ones.
– Acorn
2 days ago
10
@Lundin It seems you don't understand how this approach works. If you have to add another case, there is no need to change anything else. That is the entire point.
– Acorn
2 days ago
|
show 8 more comments
up vote
9
down vote
If you have the same signature for your function you can do something like this:
bool Do1(void) { printf("function %sn", __func__); return true;}
bool Do2(void) { printf("function %sn", __func__); return true;}
bool Do3(void) { printf("function %sn", __func__); return false;}
bool Do4(void) { printf("function %sn", __func__); return true;}
bool Do5(void) { printf("function %sn", __func__); return true;}
void Undo1(void) { printf("function %sn", __func__);}
void Undo2(void) { printf("function %sn", __func__);}
void Undo3(void) { printf("function %sn", __func__);}
void Undo4(void) { printf("function %sn", __func__);}
void Undo5(void) { printf("function %sn", __func__);}
typedef struct action {
bool (*Do)(void);
void (*Undo)(void);
} action_s;
int main(void)
{
action_s actions = {{Do1, Undo1},
{Do2, Undo2},
{Do3, Undo3},
{Do4, Undo4},
{Do5, Undo5},
{NULL, NULL}};
for (size_t i = 0; actions[i].Do; ++i) {
if (!actions[i].Do()) {
printf("Failed %zu.n", i + 1);
for (int j = i - 1; j >= 0; --j) {
actions[j].Undo();
}
return (i);
}
}
return (0);
}
You can change the return of one of Do functions to see how it react :)
Comments are not for extended discussion; this conversation has been moved to chat.
– Samuel Liew♦
yesterday
add a comment |
up vote
8
down vote
This might be one case for using gotos.
Sure, let's try that. Here's a possible implementation:
#include "stdio.h"
int main(int argc, char **argv) {
int errorCode = 0;
if (Do1()) { errorCode = 1; goto undo_0; }
if (Do2()) { errorCode = 2; goto undo_1; }
if (Do3()) { errorCode = 3; goto undo_2; }
if (Do4()) { errorCode = 4; goto undo_3; }
if (Do5()) { errorCode = 5; goto undo_4; }
undo_5: Undo5(); /* deliberate fallthrough */
undo_4: Undo4();
undo_3: Undo3();
undo_2: Undo2();
undo_1: Undo1();
undo_0: /* nothing to undo in this case */
if (errorCode != 0) {
printf("Failed %dn", errorCode);
return errorCode;
}
return 0;
}
2
The -1 is probablky from someone who considersgoto
as absolutely evil, which it is not as long as it's abused.
– Jabberwocky
2 days ago
1
@Aconcagua It is rare to be able to factorize theprintf
, and even if you can, it is not typically done. In a "generic" example like this, you shouldn't write it like that, because beginners will think it is the common way of doing it, in my opinion. Also, there is the point about the initialization to-1
and also not talking about cases where you may have to return early (i.e. before theUndo
s). Finally, better answers are currently lower than this one.
– Acorn
2 days ago
2
@Lundin It isn't.
– Acorn
2 days ago
1
@Acorn The error behavior of every single error code is tightly coupled with the error behavior of every other error code. So if you add something in the middle during maintenance, it all comes crashing down.
– Lundin
2 days ago
3
@Lundin It isn't tied, at all. You are making assumptions, and they do not even correspond to actual code out there.
– Acorn
2 days ago
|
show 8 more comments
up vote
7
down vote
For completeness a bit of obfuscation:
int foo(void)
{
int rc;
if (0
|| (rc = 1, do1())
|| (rc = 2, do2())
|| (rc = 3, do3())
|| (rc = 4, do4())
|| (rc = 5, do5())
|| (rc = 0)
)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
5
Please, stop this madness.
– Acorn
2 days ago
3
@Acorn: Why? Nice example for how to use the comma-operator ... ;-)
– alk
2 days ago
1
@Acorn: Also nicely symmetrical and compact, perfect to auto-generate.
– alk
2 days ago
2
The other solutions are just as easy to auto-generate (if you are talking about code generation).
– Acorn
2 days ago
13
Now all we are missing is a version with Duff's device and we'll be ready to submit this thread to IOCCC :)
– Lundin
2 days ago
|
show 3 more comments
up vote
3
down vote
Use goto
to manage cleanup in C.
For instance, check the Linux kernel coding style:
The rationale for using
goto
s is:
- unconditional statements are easier to understand and follow nesting is reduced
- errors by not updating individual exit points when making modifications are prevented
- saves the compiler work to optimize redundant code away ;)
Example:
int fun(int a)
{
int result = 0;
char *buffer;
buffer = kmalloc(SIZE, GFP_KERNEL);
if (!buffer)
return -ENOMEM;
if (condition1) {
while (loop1) {
...
}
result = 1;
goto out_free_buffer;
}
...
out_free_buffer:
kfree(buffer);
return result;
}
In your particular case, it could look like:
int f(...)
{
int ret;
if (Do1()) {
printf("Failed 1");
ret = 1;
goto undo1;
}
...
if (Do5()) {
printf("Failed 5");
ret = 5;
goto undo5;
}
// all good, return here if you need to keep the resources
// (or not, if you want them deallocated; in that case initialize `ret`)
return 0;
undo5:
Undo4();
...
undo1:
return ret;
}
2
This is somewhat acceptable use of goto - it is a pattern from BASIC known as "on error goto". I wish people wouldn't stop thinking there, but think one step further still. The better alternative to "on error goto" is to use a wrapper function and from the inner functionreturn code;
upon error. Leave resource allocation and clean-up to the outer wrapper. Thus separate resource allocation and algorithm in 2 different functions. Much cleaner design, easier to read and no goto debate. In general, I would recommend staying away from "the Linux kernel coding style" document.
– Lundin
2 days ago
@Lundin Not sure, are you now referring Tom's function pointer solution (your description sounds somehow different to me...)? If not, how would then these wrapper functions look like? Cannot think of anything better thanint callDo2(void) { if(do2()) { undo1(); return 2; } return callDo3(); }
at the moment, but cannot imagine either that you really meant such ones...
– Aconcagua
2 days ago
1
Basically replace all your goto with return, then in the outer wrapper function call "undo" based on what the function returned.
– Lundin
2 days ago
@Lundin Please show an example, because it does not sound like a good idea at all.
– Acorn
2 days ago
add a comment |
up vote
2
down vote
There are probably many ways to do this, but one idea is since you won't call one function unless the preceeding one succeeded, you could chain your function calls using else if
like this. And using a variable to track where it fails you can use a switch
statement to roll back easily too.
int ret=0;
if(Do1()) {
ret=1;
} else if(Do2()) {
ret=2;
} else if(Do3()) {
ret=3;
} else if(Do4()) {
ret=4;
} else if(Do5()) {
ret=5;
}
switch(ret) {
case 5:
Undo4();
case 4:
Undo3();
case 3:
Undo2();
case 2:
Undo1();
case 1:
printf("Failed %dn",ret);
break;
}
return ret;
8
Don't do this. The code is harder to read and doing more branches compared to simplegoto
.
– Acorn
2 days ago
4
Also, note it is wrong: ifDo5()
fails, we don't want to runUndo5()
(typically).
– Acorn
2 days ago
1
switch(ret)
should beswitch(ret-1)
. Also an (emtpy)default
case would be nice. All in all I like this approach.
– alk
2 days ago
@alk Writing the actual correct values would be better -- if you are keen on using this solution, which you should not ;) As for thedefault
case, what for?
– Acorn
2 days ago
6
The lengths people will go to avoid uncoditional jump which was basically kept in C language exactly for such cases because it makes it more readable than arrow antipattern or switch/if ladder is amazing... Take my downvote.
– Purple Ice
2 days ago
|
show 8 more comments
up vote
1
down vote
Yes, as explained by other answers, using goto
for error-handling is often appropriate in C.
That said, if possible, you probably should make your cleanup code safe to call even if the corresponding action was never performed. For example, instead of:
void foo()
{
int result;
int* p = malloc(...);
if (p == NULL) { result = 1; goto err1; }
int* p2 = malloc(...);
if (p2 == NULL) { result = 2; goto err2; }
int* p3 = malloc(...);
if (p3 == NULL) { result = 3; goto err3; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Maybe we don't need p3 anymore.
free(p3);
return 0;
err3:
free(p3);
err2:
free(p2);
err1:
free(p1);
return result;
}
I'd advocate:
void foo()
{
int result = -1; // Or some generic error code for unknown errors.
int* p = NULL;
int* p2 = NULL;
int* p3 = NULL;
p = malloc(...);
if (p == NULL) { result = 1; goto exit; }
p2 = malloc(...);
if (p2 == NULL) { result = 2; goto exit; }
p3 = malloc(...);
if (p3 == NULL) { result = 3; goto exit; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Set success *only* on the successful path.
result = 0;
exit:
// free(NULL) is a no-op, so this is safe even if p3 was never allocated.
free(p3);
if (result != 0)
{
free(p2);
free(p1);
}
return result;
}
It's slightly less efficient since it requires initializing variables to NULL
, but it's more maintainable since you don't need extra labels. There's less stuff to get wrong when making changes to the code. Also, if there's cleanup code that you need on both success and failure paths, you can avoid code duplication.
add a comment |
up vote
0
down vote
I typically approach this kind of problem by nesting the conditionals:
int rval = 1;
if (!Do1()) {
if (!Do2()) {
if (!Do3()) {
if (!Do4()) {
if (!Do5()) {
return 0;
// or "goto succeeded", or ...;
} else {
printf("Failed 5");
rval = 5;
}
Undo4();
} else {
printf("Failed 4");
rval = 4;
}
Undo3();
} else {
printf("Failed 3");
rval = 3;
}
Undo2();
} else {
printf("Failed 2");
rval = 2;
}
Undo1();
} else {
printf("Failed 1");
rval = 1;
}
return rval;
Usually, for me, the DoX()
are some kind of resource acquisition, such as malloc()
, and the UndoX()
are corresponding resource releases that should be performed only in the event of failure. The nesting clearly shows the association between corresponding acquisitions and releases, and avoids the need for repetition of the code for undo operations. It's also very easy to write -- you don't need to create or maintain labels, and it's easy to put the resource release in the right place as soon as you write the acquisition.
This approach does sometimes produce deeply nested code. That doesn't bother me much, but you might consider it an issue.
2
'deeply nested' - plus error and error handling get separated by quite some distance, the more for the less deeply nested ones...
– Aconcagua
yesterday
@Aconcagua, separation of error and error-handling is a pretty consistent characteristic of all of these approaches. It follows directly from avoiding repetition of the error-handling code.
– John Bollinger
yesterday
add a comment |
up vote
0
down vote
Here is an answer that I have found resilient to bugs.
Yes. It uses goto
. I firmly believe you should use what gives you most clarity, rather than just blindly following the advice of those before you (goto
as a construct can make spaghetti code, but in this instance every other error handling method ususally ends up more spaghetti-like than using this method of goto
, so IMO it's superior).
Some people may not like the form of this code, but I contest that when used to the style it is cleaner, easier to read (when everything's lined up, of course), and much more resilient to errors. If you have the properly linter/static analysis setup, and you're working with POSIX, it pretty much requires you to code in this fashion to allow for good error handling.
static char *readbuf(char *path)
{
struct stat st;
char *s = NULL;
size_t size = 0;
int fd = -1;
if (!path) { return NULL; }
if ((stat(path, &st)) < 0) { perror(path); goto _throw; }
size = st.st_size;
if (size == 0) { printf("%s is empty!n", path); goto _throw; }
if (!(s = calloc(size, 1))) { perror("calloc"); goto _throw; }
fd = open(path, O_RDONLY);
if (fd < -1) { perror(path); goto _throw; }
if ((read(fd, s, size)) < 0) { perror("read"); goto _throw; }
close(fd); /* There's really no point checking close for errors */
return s;
_throw:
if (fd > 0) close(fd);
if (s) free(s);
return NULL;
}
add a comment |
up vote
-1
down vote
If the functions return some kind of state pointer or handle (like most allocation & initialization functions would), you can quite cleanly do this without goto
by giving initial values to variables. Then you can have a single deallocation function that can handle the case where only part of the resources has been allocated.
For example:
void *mymemoryblock = NULL;
FILE *myfile = NULL;
int mysocket = -1;
bool allocate_everything()
{
mymemoryblock = malloc(1000);
if (!mymemoryblock)
{
return false;
}
myfile = fopen("/file", "r");
if (!myfile)
{
return false;
}
mysocket = socket(AF_INET, SOCK_STREAM, 0);
if (mysocket < 0)
{
return false;
}
return true;
}
void deallocate_everything()
{
if (mysocket >= 0)
{
close(mysocket);
mysocket = -1;
}
if (myfile)
{
fclose(myfile);
myfile = NULL;
}
if (mymemoryblock)
{
free(mymemoryblock);
mymemoryblock = NULL;
}
}
And then just do:
if (allocate_everything())
{
do_the_deed();
}
deallocate_everything();
"If the functions return some kind of state pointer or handle..." Yes, but it is not the general case. Further, your solution requires 3 function for each function that allocates resources, plus global variables (or passing things around).
– Acorn
2 days ago
add a comment |
up vote
-1
down vote
TL;DR:
I believe it should be written as:
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Detailed explanation:
If nothing can be assumed what-so-ever about the function types, we can't easily use an array of function pointers, which would otherwise be the correct answer.
Assuming all function types are incompatible, then we would have to wrap in the original obscure design containing all those non-compatible functions, inside something else.
We should make something that is readable, maintainable, fast. We should avoid tight coupling, so that the undo behavior of "Do_x" doesn't depend on the undo behavior of "Do_y".
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Where do_func
is the function doing all the calls required by the algorithm, and the printf
is the UI output, separated from the algorithm logic.
do_func
would be implemented like a wrapper function around the actual function calls, handling the outcome depending on the result:
(With gcc -O3, do_func
is inlined in the caller, so there is no overhead for having 2 separate functions)
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
Here the specific behavior is controlled by the array undo
, which is a wrapper around the various non-compatible functions. Which functions to to call, in which order, is all part of the specific behavior tied to each result code.
We need to tidy it all up, so that we can couple a certain behavior to a certain result code. Then when needed, we only change the code in one single place if the behavior should be changed during maintenance:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
MCVE:
#include <stdbool.h>
#include <stdio.h>
// some nonsense functions:
bool Do1 (void) { puts(__func__); return false; }
bool Do2 (void) { puts(__func__); return false; }
bool Do3 (void) { puts(__func__); return false; }
bool Do4 (void) { puts(__func__); return false; }
bool Do5 (void) { puts(__func__); return true; }
void Undo1 (void) { puts(__func__); }
void Undo2 (void) { puts(__func__); }
void Undo3 (void) { puts(__func__); }
void Undo4 (void) { puts(__func__); }
void Undo5 (void) { puts(__func__); }
// wrappers specifying undo behavior:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Output:
Do1
Do2
Do3
Do4
Do5
Undo4
Undo3
Undo2
Undo1
Failed 5
12
So, for each single function in your code that allocates resources, you are going to write 4 functions instead (which, some of them duplicateUndoN()
calls in turn). Plus a wrapper. Plus a type. Plus a global array of that type. No further comments.
– Acorn
2 days ago
@Acorn No, for every ADT in my code, I'm going to write a user-friend API, a clarity of who's responsible for allocation/deallocation, private encapsulation, modular, maintainable code, without tight coupling, where everything is autonomous. In this particular case, it looks like we are patching up some old, broken design, in which case you need wrappers - which is the fault of the original (lack of) design. Of course you don't need that level of abstraction if it's just some quick & dirty hack, but in that case all that is best practices is thrown out the window anyway.
– Lundin
14 hours ago
add a comment |
up vote
-2
down vote
typedef void(*undoer)();
int undo( undoer*const* list ) {
while(*list) {
(*list)();
++list;
}
}
void undo_push( undoer** list, undoer* undo ) {
if (!undo) return;
// swap
undoer* tmp = *list;
*list = undo;
undo = tmp;
undo_push( list+1, undo );
}
int func() {
undoer undoers[6]={0};
if (Do1()) { printf("Failed 1"); return 1; }
undo_push( undoers, Undo1 );
if (Do2()) { undo(undoers); printf("Failed 2"); return 2; }
undo_push( undoers, Undo2 );
if (Do3()) { undo(undoers); printf("Failed 3"); return 3; }
undo_push( undoers, Undo3 );
if (Do4()) { undo(undoers); printf("Failed 4"); return 4; }
undo_push( undoers, Undo4 );
if (Do5()) { undo(undoers); printf("Failed 5"); return 5; }
return 6;
}
I made undo_push
do the O(n) work. This is less efficient than having undo
do the O(n) work, as we expect more push's than undos. But this version was a touch simpler.
A more industrial strength version would have head and tail pointers and even capacity.
The basic idea is to keep a queue of undo actions in a stack, then execute them if you need to clean up.
Everything is local here, so we don't pollute global state.
struct undoer {
void(*action)(void*);
void(*cleanup)(void*);
void* state;
};
struct undoers {
undoer* top;
undoer buff[5];
};
void undo( undoers u ) {
while (u.top != buff)
{
(u.top->action)(u.top->state);
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pundo(void* pu) {
undo( *(undoers*)pu );
free(pu);
}
void cleanup_undoers(undoers u) {
while (u.top != buff)
{
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pcleanup_undoers(void* pu) {
cleanup_undoers(*(undoers*)pu);
free(pu);
}
void push_undoer( undoers* to_here, undoer u ) {
if (to_here->top != (to_here->buff+5))
{
to_here->top = u;
++(to_here->top)
return;
}
undoers* chain = (undoers*)malloc( sizeof(undoers) );
memcpy(chain, to_here, sizeof(undoers));
memset(to_here, 0, sizeof(undoers));
undoer chainer;
chainer.action = pundo;
chainer.cleanup = pcleanup_undoers;
chainer.data = chain;
push_undoer( to_here, chainer );
push_undoer( to_here, u );
}
void paction( void* p ) {
(void)(*a)() = ((void)(*)());
a();
}
void push_undo( undoers* to_here, void(*action)() ) {
undor u;
u.action = paction;
u.cleanup = 0;
u.data = (void*)action;
push_undoer(to_here, u);
}
then you get:
int func() {
undoers u={0};
if (Do1()) { printf("Failed 1"); return 1; }
push_undo( &u, Undo1 );
if (Do2()) { undo(u); printf("Failed 2"); return 2; }
push_undo( &u, Undo2 );
if (Do3()) { undo(u); printf("Failed 3"); return 3; }
push_undo( &u, Undo3 );
if (Do4()) { undo(u); printf("Failed 4"); return 4; }
push_undo( &u, Undo4 );
if (Do5()) { undo(u); printf("Failed 5"); return 5; }
cleanup_undoers(u);
return 6;
}
but that is getting ridiculous.
1
More complex, requires thatDoN
/UndoN
are actual functions (and the same signature), requires stack space, slower.
– Acorn
2 days ago
add a comment |
up vote
-4
down vote
Let's try for something with zero curly braces:
int result;
result = Do1() ? 1 : 0;
result = result ? result : Do2() ? 2 : 0;
result = result ? result : Do3() ? 3 : 0;
result = result ? result : Do4() ? 4 : 0;
result = result ? result : Do5() ? 5 : 0;
result > 4 ? (Undo5(),0) : 0;
result > 3 ? Undo4() : 0;
result > 2 ? Undo3() : 0;
result > 1 ? Undo2() : 0;
result > 0 ? Undo1() : 0;
result ? printf("Failed %drn", result) : 0;
Yes. 0;
is a valid statement in C (and C++). In the case that some of the functions return something that is incompatible with this syntax (e.g. void perhaps) then the Undo5() style can be used.
This assumes theUndoN
functions return values, when in fact they may be (and most probably are) declaredvoid
(or aren't even functions at all).
– Cássio Renan
2 days ago
msvc is never a particularly standards compliant compiler, but without thinking about it I did actually develop this with void Undo functions. No idea if its actually valid. If it isn't one could just go with: `result > 4 ? (Undo5(), 0) : 0; Doesn't help of course. if 'UndoX' isn't actually a function.
– Chris Becke
2 days ago
yeah, MSVC is a bad, bad boy, for C++ at least. In C, this seems to be valid. My bad.
– Cássio Renan
2 days ago
2
I would argue thatif (result > 4) Undo5();
is easier to understand than a ternary conditional with no false action and a discarded result. (if
statements don't need curly braces)
– pizzapants184
2 days ago
True. I really was avoiding any kind of explicit flow control. ternary conditions are cheating I know.
– Chris Becke
yesterday
add a comment |
up vote
-6
down vote
A sane (no gotos, no nested or chained ifs) approach would be
int bar(void)
{
int rc = 0;
do
{
if (do1())
{
rc = 1;
break;
}
if (do2())
{
rc = 2;
break;
}
...
if (do5())
{
rc = 5;
break;
}
} while (0);
if (rc)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
10
If your definition of "sane" is "no goto" then you already failed because sanest way to handle this is indeed to use goto.
– Purple Ice
2 days ago
@PurpleIce: You are probably right for simple cases like the OP's. But the moment we have several such things woven into each other using goto is far to error prone. And yes, this latter case could be considered a design issue.
– alk
2 days ago
3
@alk Thegoto
solution scales linearly to any complexity, as shown in other answers, just like this one, but with less clutter and extraneous loops and branches.
– Acorn
2 days ago
3
Thedo { ... } while (0)
used here is just an obfuscated way of writing agoto
. There’s no advantage at all compared to usinggoto
, and it’s quite a bit harder to read.
– NobodyNada
2 days ago
add a comment |
14 Answers
14
active
oldest
votes
14 Answers
14
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
26
down vote
Yes, it's quite common to use goto in such cases to avoid repeating yourself.
An example:
int hello() {
int result;
if (Do1()) { result = 1; goto err_one; }
if (Do2()) { result = 2; goto err_two; }
if (Do3()) { result = 3; goto err_three; }
if (Do4()) { result = 4; goto err_four; }
if (Do5()) { result = 5; goto err_five; }
// Assuming you'd like to return 0 on success.
return 0;
err_five:
Undo4();
err_four:
Undo3();
err_three:
Undo2();
err_two:
Undo1();
err_one:
printf("Failed %i", result);
return result;
}
As always you probably also want to keep your functions small and batch together the operations in a meaningful manner to avoid a large "undo-code".
1
Note:return 0
may or may not be needed, depending on what the function is supposed to do.
– Acorn
2 days ago
1
@Acorn Without, code wouldn't be equivalent to code presented in question, which undoes only on error...
– Aconcagua
2 days ago
3
@Lundin I agree. This answer assumes that the Do and Undo functions might have different signatures in reality which is probably more common in practice.
– likle
2 days ago
10
@Lundin There is no code repetition here. Please, don't misunderstand generic examples with concrete ones.
– Acorn
2 days ago
10
@Lundin It seems you don't understand how this approach works. If you have to add another case, there is no need to change anything else. That is the entire point.
– Acorn
2 days ago
|
show 8 more comments
up vote
26
down vote
Yes, it's quite common to use goto in such cases to avoid repeating yourself.
An example:
int hello() {
int result;
if (Do1()) { result = 1; goto err_one; }
if (Do2()) { result = 2; goto err_two; }
if (Do3()) { result = 3; goto err_three; }
if (Do4()) { result = 4; goto err_four; }
if (Do5()) { result = 5; goto err_five; }
// Assuming you'd like to return 0 on success.
return 0;
err_five:
Undo4();
err_four:
Undo3();
err_three:
Undo2();
err_two:
Undo1();
err_one:
printf("Failed %i", result);
return result;
}
As always you probably also want to keep your functions small and batch together the operations in a meaningful manner to avoid a large "undo-code".
1
Note:return 0
may or may not be needed, depending on what the function is supposed to do.
– Acorn
2 days ago
1
@Acorn Without, code wouldn't be equivalent to code presented in question, which undoes only on error...
– Aconcagua
2 days ago
3
@Lundin I agree. This answer assumes that the Do and Undo functions might have different signatures in reality which is probably more common in practice.
– likle
2 days ago
10
@Lundin There is no code repetition here. Please, don't misunderstand generic examples with concrete ones.
– Acorn
2 days ago
10
@Lundin It seems you don't understand how this approach works. If you have to add another case, there is no need to change anything else. That is the entire point.
– Acorn
2 days ago
|
show 8 more comments
up vote
26
down vote
up vote
26
down vote
Yes, it's quite common to use goto in such cases to avoid repeating yourself.
An example:
int hello() {
int result;
if (Do1()) { result = 1; goto err_one; }
if (Do2()) { result = 2; goto err_two; }
if (Do3()) { result = 3; goto err_three; }
if (Do4()) { result = 4; goto err_four; }
if (Do5()) { result = 5; goto err_five; }
// Assuming you'd like to return 0 on success.
return 0;
err_five:
Undo4();
err_four:
Undo3();
err_three:
Undo2();
err_two:
Undo1();
err_one:
printf("Failed %i", result);
return result;
}
As always you probably also want to keep your functions small and batch together the operations in a meaningful manner to avoid a large "undo-code".
Yes, it's quite common to use goto in such cases to avoid repeating yourself.
An example:
int hello() {
int result;
if (Do1()) { result = 1; goto err_one; }
if (Do2()) { result = 2; goto err_two; }
if (Do3()) { result = 3; goto err_three; }
if (Do4()) { result = 4; goto err_four; }
if (Do5()) { result = 5; goto err_five; }
// Assuming you'd like to return 0 on success.
return 0;
err_five:
Undo4();
err_four:
Undo3();
err_three:
Undo2();
err_two:
Undo1();
err_one:
printf("Failed %i", result);
return result;
}
As always you probably also want to keep your functions small and batch together the operations in a meaningful manner to avoid a large "undo-code".
edited 2 days ago
answered 2 days ago
likle
91519
91519
1
Note:return 0
may or may not be needed, depending on what the function is supposed to do.
– Acorn
2 days ago
1
@Acorn Without, code wouldn't be equivalent to code presented in question, which undoes only on error...
– Aconcagua
2 days ago
3
@Lundin I agree. This answer assumes that the Do and Undo functions might have different signatures in reality which is probably more common in practice.
– likle
2 days ago
10
@Lundin There is no code repetition here. Please, don't misunderstand generic examples with concrete ones.
– Acorn
2 days ago
10
@Lundin It seems you don't understand how this approach works. If you have to add another case, there is no need to change anything else. That is the entire point.
– Acorn
2 days ago
|
show 8 more comments
1
Note:return 0
may or may not be needed, depending on what the function is supposed to do.
– Acorn
2 days ago
1
@Acorn Without, code wouldn't be equivalent to code presented in question, which undoes only on error...
– Aconcagua
2 days ago
3
@Lundin I agree. This answer assumes that the Do and Undo functions might have different signatures in reality which is probably more common in practice.
– likle
2 days ago
10
@Lundin There is no code repetition here. Please, don't misunderstand generic examples with concrete ones.
– Acorn
2 days ago
10
@Lundin It seems you don't understand how this approach works. If you have to add another case, there is no need to change anything else. That is the entire point.
– Acorn
2 days ago
1
1
Note:
return 0
may or may not be needed, depending on what the function is supposed to do.– Acorn
2 days ago
Note:
return 0
may or may not be needed, depending on what the function is supposed to do.– Acorn
2 days ago
1
1
@Acorn Without, code wouldn't be equivalent to code presented in question, which undoes only on error...
– Aconcagua
2 days ago
@Acorn Without, code wouldn't be equivalent to code presented in question, which undoes only on error...
– Aconcagua
2 days ago
3
3
@Lundin I agree. This answer assumes that the Do and Undo functions might have different signatures in reality which is probably more common in practice.
– likle
2 days ago
@Lundin I agree. This answer assumes that the Do and Undo functions might have different signatures in reality which is probably more common in practice.
– likle
2 days ago
10
10
@Lundin There is no code repetition here. Please, don't misunderstand generic examples with concrete ones.
– Acorn
2 days ago
@Lundin There is no code repetition here. Please, don't misunderstand generic examples with concrete ones.
– Acorn
2 days ago
10
10
@Lundin It seems you don't understand how this approach works. If you have to add another case, there is no need to change anything else. That is the entire point.
– Acorn
2 days ago
@Lundin It seems you don't understand how this approach works. If you have to add another case, there is no need to change anything else. That is the entire point.
– Acorn
2 days ago
|
show 8 more comments
up vote
9
down vote
If you have the same signature for your function you can do something like this:
bool Do1(void) { printf("function %sn", __func__); return true;}
bool Do2(void) { printf("function %sn", __func__); return true;}
bool Do3(void) { printf("function %sn", __func__); return false;}
bool Do4(void) { printf("function %sn", __func__); return true;}
bool Do5(void) { printf("function %sn", __func__); return true;}
void Undo1(void) { printf("function %sn", __func__);}
void Undo2(void) { printf("function %sn", __func__);}
void Undo3(void) { printf("function %sn", __func__);}
void Undo4(void) { printf("function %sn", __func__);}
void Undo5(void) { printf("function %sn", __func__);}
typedef struct action {
bool (*Do)(void);
void (*Undo)(void);
} action_s;
int main(void)
{
action_s actions = {{Do1, Undo1},
{Do2, Undo2},
{Do3, Undo3},
{Do4, Undo4},
{Do5, Undo5},
{NULL, NULL}};
for (size_t i = 0; actions[i].Do; ++i) {
if (!actions[i].Do()) {
printf("Failed %zu.n", i + 1);
for (int j = i - 1; j >= 0; --j) {
actions[j].Undo();
}
return (i);
}
}
return (0);
}
You can change the return of one of Do functions to see how it react :)
Comments are not for extended discussion; this conversation has been moved to chat.
– Samuel Liew♦
yesterday
add a comment |
up vote
9
down vote
If you have the same signature for your function you can do something like this:
bool Do1(void) { printf("function %sn", __func__); return true;}
bool Do2(void) { printf("function %sn", __func__); return true;}
bool Do3(void) { printf("function %sn", __func__); return false;}
bool Do4(void) { printf("function %sn", __func__); return true;}
bool Do5(void) { printf("function %sn", __func__); return true;}
void Undo1(void) { printf("function %sn", __func__);}
void Undo2(void) { printf("function %sn", __func__);}
void Undo3(void) { printf("function %sn", __func__);}
void Undo4(void) { printf("function %sn", __func__);}
void Undo5(void) { printf("function %sn", __func__);}
typedef struct action {
bool (*Do)(void);
void (*Undo)(void);
} action_s;
int main(void)
{
action_s actions = {{Do1, Undo1},
{Do2, Undo2},
{Do3, Undo3},
{Do4, Undo4},
{Do5, Undo5},
{NULL, NULL}};
for (size_t i = 0; actions[i].Do; ++i) {
if (!actions[i].Do()) {
printf("Failed %zu.n", i + 1);
for (int j = i - 1; j >= 0; --j) {
actions[j].Undo();
}
return (i);
}
}
return (0);
}
You can change the return of one of Do functions to see how it react :)
Comments are not for extended discussion; this conversation has been moved to chat.
– Samuel Liew♦
yesterday
add a comment |
up vote
9
down vote
up vote
9
down vote
If you have the same signature for your function you can do something like this:
bool Do1(void) { printf("function %sn", __func__); return true;}
bool Do2(void) { printf("function %sn", __func__); return true;}
bool Do3(void) { printf("function %sn", __func__); return false;}
bool Do4(void) { printf("function %sn", __func__); return true;}
bool Do5(void) { printf("function %sn", __func__); return true;}
void Undo1(void) { printf("function %sn", __func__);}
void Undo2(void) { printf("function %sn", __func__);}
void Undo3(void) { printf("function %sn", __func__);}
void Undo4(void) { printf("function %sn", __func__);}
void Undo5(void) { printf("function %sn", __func__);}
typedef struct action {
bool (*Do)(void);
void (*Undo)(void);
} action_s;
int main(void)
{
action_s actions = {{Do1, Undo1},
{Do2, Undo2},
{Do3, Undo3},
{Do4, Undo4},
{Do5, Undo5},
{NULL, NULL}};
for (size_t i = 0; actions[i].Do; ++i) {
if (!actions[i].Do()) {
printf("Failed %zu.n", i + 1);
for (int j = i - 1; j >= 0; --j) {
actions[j].Undo();
}
return (i);
}
}
return (0);
}
You can change the return of one of Do functions to see how it react :)
If you have the same signature for your function you can do something like this:
bool Do1(void) { printf("function %sn", __func__); return true;}
bool Do2(void) { printf("function %sn", __func__); return true;}
bool Do3(void) { printf("function %sn", __func__); return false;}
bool Do4(void) { printf("function %sn", __func__); return true;}
bool Do5(void) { printf("function %sn", __func__); return true;}
void Undo1(void) { printf("function %sn", __func__);}
void Undo2(void) { printf("function %sn", __func__);}
void Undo3(void) { printf("function %sn", __func__);}
void Undo4(void) { printf("function %sn", __func__);}
void Undo5(void) { printf("function %sn", __func__);}
typedef struct action {
bool (*Do)(void);
void (*Undo)(void);
} action_s;
int main(void)
{
action_s actions = {{Do1, Undo1},
{Do2, Undo2},
{Do3, Undo3},
{Do4, Undo4},
{Do5, Undo5},
{NULL, NULL}};
for (size_t i = 0; actions[i].Do; ++i) {
if (!actions[i].Do()) {
printf("Failed %zu.n", i + 1);
for (int j = i - 1; j >= 0; --j) {
actions[j].Undo();
}
return (i);
}
}
return (0);
}
You can change the return of one of Do functions to see how it react :)
edited 2 days ago
Peter Mortensen
13.3k1983111
13.3k1983111
answered 2 days ago
Tom's
1,842419
1,842419
Comments are not for extended discussion; this conversation has been moved to chat.
– Samuel Liew♦
yesterday
add a comment |
Comments are not for extended discussion; this conversation has been moved to chat.
– Samuel Liew♦
yesterday
Comments are not for extended discussion; this conversation has been moved to chat.
– Samuel Liew♦
yesterday
Comments are not for extended discussion; this conversation has been moved to chat.
– Samuel Liew♦
yesterday
add a comment |
up vote
8
down vote
This might be one case for using gotos.
Sure, let's try that. Here's a possible implementation:
#include "stdio.h"
int main(int argc, char **argv) {
int errorCode = 0;
if (Do1()) { errorCode = 1; goto undo_0; }
if (Do2()) { errorCode = 2; goto undo_1; }
if (Do3()) { errorCode = 3; goto undo_2; }
if (Do4()) { errorCode = 4; goto undo_3; }
if (Do5()) { errorCode = 5; goto undo_4; }
undo_5: Undo5(); /* deliberate fallthrough */
undo_4: Undo4();
undo_3: Undo3();
undo_2: Undo2();
undo_1: Undo1();
undo_0: /* nothing to undo in this case */
if (errorCode != 0) {
printf("Failed %dn", errorCode);
return errorCode;
}
return 0;
}
2
The -1 is probablky from someone who considersgoto
as absolutely evil, which it is not as long as it's abused.
– Jabberwocky
2 days ago
1
@Aconcagua It is rare to be able to factorize theprintf
, and even if you can, it is not typically done. In a "generic" example like this, you shouldn't write it like that, because beginners will think it is the common way of doing it, in my opinion. Also, there is the point about the initialization to-1
and also not talking about cases where you may have to return early (i.e. before theUndo
s). Finally, better answers are currently lower than this one.
– Acorn
2 days ago
2
@Lundin It isn't.
– Acorn
2 days ago
1
@Acorn The error behavior of every single error code is tightly coupled with the error behavior of every other error code. So if you add something in the middle during maintenance, it all comes crashing down.
– Lundin
2 days ago
3
@Lundin It isn't tied, at all. You are making assumptions, and they do not even correspond to actual code out there.
– Acorn
2 days ago
|
show 8 more comments
up vote
8
down vote
This might be one case for using gotos.
Sure, let's try that. Here's a possible implementation:
#include "stdio.h"
int main(int argc, char **argv) {
int errorCode = 0;
if (Do1()) { errorCode = 1; goto undo_0; }
if (Do2()) { errorCode = 2; goto undo_1; }
if (Do3()) { errorCode = 3; goto undo_2; }
if (Do4()) { errorCode = 4; goto undo_3; }
if (Do5()) { errorCode = 5; goto undo_4; }
undo_5: Undo5(); /* deliberate fallthrough */
undo_4: Undo4();
undo_3: Undo3();
undo_2: Undo2();
undo_1: Undo1();
undo_0: /* nothing to undo in this case */
if (errorCode != 0) {
printf("Failed %dn", errorCode);
return errorCode;
}
return 0;
}
2
The -1 is probablky from someone who considersgoto
as absolutely evil, which it is not as long as it's abused.
– Jabberwocky
2 days ago
1
@Aconcagua It is rare to be able to factorize theprintf
, and even if you can, it is not typically done. In a "generic" example like this, you shouldn't write it like that, because beginners will think it is the common way of doing it, in my opinion. Also, there is the point about the initialization to-1
and also not talking about cases where you may have to return early (i.e. before theUndo
s). Finally, better answers are currently lower than this one.
– Acorn
2 days ago
2
@Lundin It isn't.
– Acorn
2 days ago
1
@Acorn The error behavior of every single error code is tightly coupled with the error behavior of every other error code. So if you add something in the middle during maintenance, it all comes crashing down.
– Lundin
2 days ago
3
@Lundin It isn't tied, at all. You are making assumptions, and they do not even correspond to actual code out there.
– Acorn
2 days ago
|
show 8 more comments
up vote
8
down vote
up vote
8
down vote
This might be one case for using gotos.
Sure, let's try that. Here's a possible implementation:
#include "stdio.h"
int main(int argc, char **argv) {
int errorCode = 0;
if (Do1()) { errorCode = 1; goto undo_0; }
if (Do2()) { errorCode = 2; goto undo_1; }
if (Do3()) { errorCode = 3; goto undo_2; }
if (Do4()) { errorCode = 4; goto undo_3; }
if (Do5()) { errorCode = 5; goto undo_4; }
undo_5: Undo5(); /* deliberate fallthrough */
undo_4: Undo4();
undo_3: Undo3();
undo_2: Undo2();
undo_1: Undo1();
undo_0: /* nothing to undo in this case */
if (errorCode != 0) {
printf("Failed %dn", errorCode);
return errorCode;
}
return 0;
}
This might be one case for using gotos.
Sure, let's try that. Here's a possible implementation:
#include "stdio.h"
int main(int argc, char **argv) {
int errorCode = 0;
if (Do1()) { errorCode = 1; goto undo_0; }
if (Do2()) { errorCode = 2; goto undo_1; }
if (Do3()) { errorCode = 3; goto undo_2; }
if (Do4()) { errorCode = 4; goto undo_3; }
if (Do5()) { errorCode = 5; goto undo_4; }
undo_5: Undo5(); /* deliberate fallthrough */
undo_4: Undo4();
undo_3: Undo3();
undo_2: Undo2();
undo_1: Undo1();
undo_0: /* nothing to undo in this case */
if (errorCode != 0) {
printf("Failed %dn", errorCode);
return errorCode;
}
return 0;
}
edited 2 days ago
answered 2 days ago
Blaze
2,8911524
2,8911524
2
The -1 is probablky from someone who considersgoto
as absolutely evil, which it is not as long as it's abused.
– Jabberwocky
2 days ago
1
@Aconcagua It is rare to be able to factorize theprintf
, and even if you can, it is not typically done. In a "generic" example like this, you shouldn't write it like that, because beginners will think it is the common way of doing it, in my opinion. Also, there is the point about the initialization to-1
and also not talking about cases where you may have to return early (i.e. before theUndo
s). Finally, better answers are currently lower than this one.
– Acorn
2 days ago
2
@Lundin It isn't.
– Acorn
2 days ago
1
@Acorn The error behavior of every single error code is tightly coupled with the error behavior of every other error code. So if you add something in the middle during maintenance, it all comes crashing down.
– Lundin
2 days ago
3
@Lundin It isn't tied, at all. You are making assumptions, and they do not even correspond to actual code out there.
– Acorn
2 days ago
|
show 8 more comments
2
The -1 is probablky from someone who considersgoto
as absolutely evil, which it is not as long as it's abused.
– Jabberwocky
2 days ago
1
@Aconcagua It is rare to be able to factorize theprintf
, and even if you can, it is not typically done. In a "generic" example like this, you shouldn't write it like that, because beginners will think it is the common way of doing it, in my opinion. Also, there is the point about the initialization to-1
and also not talking about cases where you may have to return early (i.e. before theUndo
s). Finally, better answers are currently lower than this one.
– Acorn
2 days ago
2
@Lundin It isn't.
– Acorn
2 days ago
1
@Acorn The error behavior of every single error code is tightly coupled with the error behavior of every other error code. So if you add something in the middle during maintenance, it all comes crashing down.
– Lundin
2 days ago
3
@Lundin It isn't tied, at all. You are making assumptions, and they do not even correspond to actual code out there.
– Acorn
2 days ago
2
2
The -1 is probablky from someone who considers
goto
as absolutely evil, which it is not as long as it's abused.– Jabberwocky
2 days ago
The -1 is probablky from someone who considers
goto
as absolutely evil, which it is not as long as it's abused.– Jabberwocky
2 days ago
1
1
@Aconcagua It is rare to be able to factorize the
printf
, and even if you can, it is not typically done. In a "generic" example like this, you shouldn't write it like that, because beginners will think it is the common way of doing it, in my opinion. Also, there is the point about the initialization to -1
and also not talking about cases where you may have to return early (i.e. before the Undo
s). Finally, better answers are currently lower than this one.– Acorn
2 days ago
@Aconcagua It is rare to be able to factorize the
printf
, and even if you can, it is not typically done. In a "generic" example like this, you shouldn't write it like that, because beginners will think it is the common way of doing it, in my opinion. Also, there is the point about the initialization to -1
and also not talking about cases where you may have to return early (i.e. before the Undo
s). Finally, better answers are currently lower than this one.– Acorn
2 days ago
2
2
@Lundin It isn't.
– Acorn
2 days ago
@Lundin It isn't.
– Acorn
2 days ago
1
1
@Acorn The error behavior of every single error code is tightly coupled with the error behavior of every other error code. So if you add something in the middle during maintenance, it all comes crashing down.
– Lundin
2 days ago
@Acorn The error behavior of every single error code is tightly coupled with the error behavior of every other error code. So if you add something in the middle during maintenance, it all comes crashing down.
– Lundin
2 days ago
3
3
@Lundin It isn't tied, at all. You are making assumptions, and they do not even correspond to actual code out there.
– Acorn
2 days ago
@Lundin It isn't tied, at all. You are making assumptions, and they do not even correspond to actual code out there.
– Acorn
2 days ago
|
show 8 more comments
up vote
7
down vote
For completeness a bit of obfuscation:
int foo(void)
{
int rc;
if (0
|| (rc = 1, do1())
|| (rc = 2, do2())
|| (rc = 3, do3())
|| (rc = 4, do4())
|| (rc = 5, do5())
|| (rc = 0)
)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
5
Please, stop this madness.
– Acorn
2 days ago
3
@Acorn: Why? Nice example for how to use the comma-operator ... ;-)
– alk
2 days ago
1
@Acorn: Also nicely symmetrical and compact, perfect to auto-generate.
– alk
2 days ago
2
The other solutions are just as easy to auto-generate (if you are talking about code generation).
– Acorn
2 days ago
13
Now all we are missing is a version with Duff's device and we'll be ready to submit this thread to IOCCC :)
– Lundin
2 days ago
|
show 3 more comments
up vote
7
down vote
For completeness a bit of obfuscation:
int foo(void)
{
int rc;
if (0
|| (rc = 1, do1())
|| (rc = 2, do2())
|| (rc = 3, do3())
|| (rc = 4, do4())
|| (rc = 5, do5())
|| (rc = 0)
)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
5
Please, stop this madness.
– Acorn
2 days ago
3
@Acorn: Why? Nice example for how to use the comma-operator ... ;-)
– alk
2 days ago
1
@Acorn: Also nicely symmetrical and compact, perfect to auto-generate.
– alk
2 days ago
2
The other solutions are just as easy to auto-generate (if you are talking about code generation).
– Acorn
2 days ago
13
Now all we are missing is a version with Duff's device and we'll be ready to submit this thread to IOCCC :)
– Lundin
2 days ago
|
show 3 more comments
up vote
7
down vote
up vote
7
down vote
For completeness a bit of obfuscation:
int foo(void)
{
int rc;
if (0
|| (rc = 1, do1())
|| (rc = 2, do2())
|| (rc = 3, do3())
|| (rc = 4, do4())
|| (rc = 5, do5())
|| (rc = 0)
)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
For completeness a bit of obfuscation:
int foo(void)
{
int rc;
if (0
|| (rc = 1, do1())
|| (rc = 2, do2())
|| (rc = 3, do3())
|| (rc = 4, do4())
|| (rc = 5, do5())
|| (rc = 0)
)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
edited 2 days ago
answered 2 days ago
alk
57.7k759167
57.7k759167
5
Please, stop this madness.
– Acorn
2 days ago
3
@Acorn: Why? Nice example for how to use the comma-operator ... ;-)
– alk
2 days ago
1
@Acorn: Also nicely symmetrical and compact, perfect to auto-generate.
– alk
2 days ago
2
The other solutions are just as easy to auto-generate (if you are talking about code generation).
– Acorn
2 days ago
13
Now all we are missing is a version with Duff's device and we'll be ready to submit this thread to IOCCC :)
– Lundin
2 days ago
|
show 3 more comments
5
Please, stop this madness.
– Acorn
2 days ago
3
@Acorn: Why? Nice example for how to use the comma-operator ... ;-)
– alk
2 days ago
1
@Acorn: Also nicely symmetrical and compact, perfect to auto-generate.
– alk
2 days ago
2
The other solutions are just as easy to auto-generate (if you are talking about code generation).
– Acorn
2 days ago
13
Now all we are missing is a version with Duff's device and we'll be ready to submit this thread to IOCCC :)
– Lundin
2 days ago
5
5
Please, stop this madness.
– Acorn
2 days ago
Please, stop this madness.
– Acorn
2 days ago
3
3
@Acorn: Why? Nice example for how to use the comma-operator ... ;-)
– alk
2 days ago
@Acorn: Why? Nice example for how to use the comma-operator ... ;-)
– alk
2 days ago
1
1
@Acorn: Also nicely symmetrical and compact, perfect to auto-generate.
– alk
2 days ago
@Acorn: Also nicely symmetrical and compact, perfect to auto-generate.
– alk
2 days ago
2
2
The other solutions are just as easy to auto-generate (if you are talking about code generation).
– Acorn
2 days ago
The other solutions are just as easy to auto-generate (if you are talking about code generation).
– Acorn
2 days ago
13
13
Now all we are missing is a version with Duff's device and we'll be ready to submit this thread to IOCCC :)
– Lundin
2 days ago
Now all we are missing is a version with Duff's device and we'll be ready to submit this thread to IOCCC :)
– Lundin
2 days ago
|
show 3 more comments
up vote
3
down vote
Use goto
to manage cleanup in C.
For instance, check the Linux kernel coding style:
The rationale for using
goto
s is:
- unconditional statements are easier to understand and follow nesting is reduced
- errors by not updating individual exit points when making modifications are prevented
- saves the compiler work to optimize redundant code away ;)
Example:
int fun(int a)
{
int result = 0;
char *buffer;
buffer = kmalloc(SIZE, GFP_KERNEL);
if (!buffer)
return -ENOMEM;
if (condition1) {
while (loop1) {
...
}
result = 1;
goto out_free_buffer;
}
...
out_free_buffer:
kfree(buffer);
return result;
}
In your particular case, it could look like:
int f(...)
{
int ret;
if (Do1()) {
printf("Failed 1");
ret = 1;
goto undo1;
}
...
if (Do5()) {
printf("Failed 5");
ret = 5;
goto undo5;
}
// all good, return here if you need to keep the resources
// (or not, if you want them deallocated; in that case initialize `ret`)
return 0;
undo5:
Undo4();
...
undo1:
return ret;
}
2
This is somewhat acceptable use of goto - it is a pattern from BASIC known as "on error goto". I wish people wouldn't stop thinking there, but think one step further still. The better alternative to "on error goto" is to use a wrapper function and from the inner functionreturn code;
upon error. Leave resource allocation and clean-up to the outer wrapper. Thus separate resource allocation and algorithm in 2 different functions. Much cleaner design, easier to read and no goto debate. In general, I would recommend staying away from "the Linux kernel coding style" document.
– Lundin
2 days ago
@Lundin Not sure, are you now referring Tom's function pointer solution (your description sounds somehow different to me...)? If not, how would then these wrapper functions look like? Cannot think of anything better thanint callDo2(void) { if(do2()) { undo1(); return 2; } return callDo3(); }
at the moment, but cannot imagine either that you really meant such ones...
– Aconcagua
2 days ago
1
Basically replace all your goto with return, then in the outer wrapper function call "undo" based on what the function returned.
– Lundin
2 days ago
@Lundin Please show an example, because it does not sound like a good idea at all.
– Acorn
2 days ago
add a comment |
up vote
3
down vote
Use goto
to manage cleanup in C.
For instance, check the Linux kernel coding style:
The rationale for using
goto
s is:
- unconditional statements are easier to understand and follow nesting is reduced
- errors by not updating individual exit points when making modifications are prevented
- saves the compiler work to optimize redundant code away ;)
Example:
int fun(int a)
{
int result = 0;
char *buffer;
buffer = kmalloc(SIZE, GFP_KERNEL);
if (!buffer)
return -ENOMEM;
if (condition1) {
while (loop1) {
...
}
result = 1;
goto out_free_buffer;
}
...
out_free_buffer:
kfree(buffer);
return result;
}
In your particular case, it could look like:
int f(...)
{
int ret;
if (Do1()) {
printf("Failed 1");
ret = 1;
goto undo1;
}
...
if (Do5()) {
printf("Failed 5");
ret = 5;
goto undo5;
}
// all good, return here if you need to keep the resources
// (or not, if you want them deallocated; in that case initialize `ret`)
return 0;
undo5:
Undo4();
...
undo1:
return ret;
}
2
This is somewhat acceptable use of goto - it is a pattern from BASIC known as "on error goto". I wish people wouldn't stop thinking there, but think one step further still. The better alternative to "on error goto" is to use a wrapper function and from the inner functionreturn code;
upon error. Leave resource allocation and clean-up to the outer wrapper. Thus separate resource allocation and algorithm in 2 different functions. Much cleaner design, easier to read and no goto debate. In general, I would recommend staying away from "the Linux kernel coding style" document.
– Lundin
2 days ago
@Lundin Not sure, are you now referring Tom's function pointer solution (your description sounds somehow different to me...)? If not, how would then these wrapper functions look like? Cannot think of anything better thanint callDo2(void) { if(do2()) { undo1(); return 2; } return callDo3(); }
at the moment, but cannot imagine either that you really meant such ones...
– Aconcagua
2 days ago
1
Basically replace all your goto with return, then in the outer wrapper function call "undo" based on what the function returned.
– Lundin
2 days ago
@Lundin Please show an example, because it does not sound like a good idea at all.
– Acorn
2 days ago
add a comment |
up vote
3
down vote
up vote
3
down vote
Use goto
to manage cleanup in C.
For instance, check the Linux kernel coding style:
The rationale for using
goto
s is:
- unconditional statements are easier to understand and follow nesting is reduced
- errors by not updating individual exit points when making modifications are prevented
- saves the compiler work to optimize redundant code away ;)
Example:
int fun(int a)
{
int result = 0;
char *buffer;
buffer = kmalloc(SIZE, GFP_KERNEL);
if (!buffer)
return -ENOMEM;
if (condition1) {
while (loop1) {
...
}
result = 1;
goto out_free_buffer;
}
...
out_free_buffer:
kfree(buffer);
return result;
}
In your particular case, it could look like:
int f(...)
{
int ret;
if (Do1()) {
printf("Failed 1");
ret = 1;
goto undo1;
}
...
if (Do5()) {
printf("Failed 5");
ret = 5;
goto undo5;
}
// all good, return here if you need to keep the resources
// (or not, if you want them deallocated; in that case initialize `ret`)
return 0;
undo5:
Undo4();
...
undo1:
return ret;
}
Use goto
to manage cleanup in C.
For instance, check the Linux kernel coding style:
The rationale for using
goto
s is:
- unconditional statements are easier to understand and follow nesting is reduced
- errors by not updating individual exit points when making modifications are prevented
- saves the compiler work to optimize redundant code away ;)
Example:
int fun(int a)
{
int result = 0;
char *buffer;
buffer = kmalloc(SIZE, GFP_KERNEL);
if (!buffer)
return -ENOMEM;
if (condition1) {
while (loop1) {
...
}
result = 1;
goto out_free_buffer;
}
...
out_free_buffer:
kfree(buffer);
return result;
}
In your particular case, it could look like:
int f(...)
{
int ret;
if (Do1()) {
printf("Failed 1");
ret = 1;
goto undo1;
}
...
if (Do5()) {
printf("Failed 5");
ret = 5;
goto undo5;
}
// all good, return here if you need to keep the resources
// (or not, if you want them deallocated; in that case initialize `ret`)
return 0;
undo5:
Undo4();
...
undo1:
return ret;
}
edited 2 days ago
answered 2 days ago
Acorn
4,67311135
4,67311135
2
This is somewhat acceptable use of goto - it is a pattern from BASIC known as "on error goto". I wish people wouldn't stop thinking there, but think one step further still. The better alternative to "on error goto" is to use a wrapper function and from the inner functionreturn code;
upon error. Leave resource allocation and clean-up to the outer wrapper. Thus separate resource allocation and algorithm in 2 different functions. Much cleaner design, easier to read and no goto debate. In general, I would recommend staying away from "the Linux kernel coding style" document.
– Lundin
2 days ago
@Lundin Not sure, are you now referring Tom's function pointer solution (your description sounds somehow different to me...)? If not, how would then these wrapper functions look like? Cannot think of anything better thanint callDo2(void) { if(do2()) { undo1(); return 2; } return callDo3(); }
at the moment, but cannot imagine either that you really meant such ones...
– Aconcagua
2 days ago
1
Basically replace all your goto with return, then in the outer wrapper function call "undo" based on what the function returned.
– Lundin
2 days ago
@Lundin Please show an example, because it does not sound like a good idea at all.
– Acorn
2 days ago
add a comment |
2
This is somewhat acceptable use of goto - it is a pattern from BASIC known as "on error goto". I wish people wouldn't stop thinking there, but think one step further still. The better alternative to "on error goto" is to use a wrapper function and from the inner functionreturn code;
upon error. Leave resource allocation and clean-up to the outer wrapper. Thus separate resource allocation and algorithm in 2 different functions. Much cleaner design, easier to read and no goto debate. In general, I would recommend staying away from "the Linux kernel coding style" document.
– Lundin
2 days ago
@Lundin Not sure, are you now referring Tom's function pointer solution (your description sounds somehow different to me...)? If not, how would then these wrapper functions look like? Cannot think of anything better thanint callDo2(void) { if(do2()) { undo1(); return 2; } return callDo3(); }
at the moment, but cannot imagine either that you really meant such ones...
– Aconcagua
2 days ago
1
Basically replace all your goto with return, then in the outer wrapper function call "undo" based on what the function returned.
– Lundin
2 days ago
@Lundin Please show an example, because it does not sound like a good idea at all.
– Acorn
2 days ago
2
2
This is somewhat acceptable use of goto - it is a pattern from BASIC known as "on error goto". I wish people wouldn't stop thinking there, but think one step further still. The better alternative to "on error goto" is to use a wrapper function and from the inner function
return code;
upon error. Leave resource allocation and clean-up to the outer wrapper. Thus separate resource allocation and algorithm in 2 different functions. Much cleaner design, easier to read and no goto debate. In general, I would recommend staying away from "the Linux kernel coding style" document.– Lundin
2 days ago
This is somewhat acceptable use of goto - it is a pattern from BASIC known as "on error goto". I wish people wouldn't stop thinking there, but think one step further still. The better alternative to "on error goto" is to use a wrapper function and from the inner function
return code;
upon error. Leave resource allocation and clean-up to the outer wrapper. Thus separate resource allocation and algorithm in 2 different functions. Much cleaner design, easier to read and no goto debate. In general, I would recommend staying away from "the Linux kernel coding style" document.– Lundin
2 days ago
@Lundin Not sure, are you now referring Tom's function pointer solution (your description sounds somehow different to me...)? If not, how would then these wrapper functions look like? Cannot think of anything better than
int callDo2(void) { if(do2()) { undo1(); return 2; } return callDo3(); }
at the moment, but cannot imagine either that you really meant such ones...– Aconcagua
2 days ago
@Lundin Not sure, are you now referring Tom's function pointer solution (your description sounds somehow different to me...)? If not, how would then these wrapper functions look like? Cannot think of anything better than
int callDo2(void) { if(do2()) { undo1(); return 2; } return callDo3(); }
at the moment, but cannot imagine either that you really meant such ones...– Aconcagua
2 days ago
1
1
Basically replace all your goto with return, then in the outer wrapper function call "undo" based on what the function returned.
– Lundin
2 days ago
Basically replace all your goto with return, then in the outer wrapper function call "undo" based on what the function returned.
– Lundin
2 days ago
@Lundin Please show an example, because it does not sound like a good idea at all.
– Acorn
2 days ago
@Lundin Please show an example, because it does not sound like a good idea at all.
– Acorn
2 days ago
add a comment |
up vote
2
down vote
There are probably many ways to do this, but one idea is since you won't call one function unless the preceeding one succeeded, you could chain your function calls using else if
like this. And using a variable to track where it fails you can use a switch
statement to roll back easily too.
int ret=0;
if(Do1()) {
ret=1;
} else if(Do2()) {
ret=2;
} else if(Do3()) {
ret=3;
} else if(Do4()) {
ret=4;
} else if(Do5()) {
ret=5;
}
switch(ret) {
case 5:
Undo4();
case 4:
Undo3();
case 3:
Undo2();
case 2:
Undo1();
case 1:
printf("Failed %dn",ret);
break;
}
return ret;
8
Don't do this. The code is harder to read and doing more branches compared to simplegoto
.
– Acorn
2 days ago
4
Also, note it is wrong: ifDo5()
fails, we don't want to runUndo5()
(typically).
– Acorn
2 days ago
1
switch(ret)
should beswitch(ret-1)
. Also an (emtpy)default
case would be nice. All in all I like this approach.
– alk
2 days ago
@alk Writing the actual correct values would be better -- if you are keen on using this solution, which you should not ;) As for thedefault
case, what for?
– Acorn
2 days ago
6
The lengths people will go to avoid uncoditional jump which was basically kept in C language exactly for such cases because it makes it more readable than arrow antipattern or switch/if ladder is amazing... Take my downvote.
– Purple Ice
2 days ago
|
show 8 more comments
up vote
2
down vote
There are probably many ways to do this, but one idea is since you won't call one function unless the preceeding one succeeded, you could chain your function calls using else if
like this. And using a variable to track where it fails you can use a switch
statement to roll back easily too.
int ret=0;
if(Do1()) {
ret=1;
} else if(Do2()) {
ret=2;
} else if(Do3()) {
ret=3;
} else if(Do4()) {
ret=4;
} else if(Do5()) {
ret=5;
}
switch(ret) {
case 5:
Undo4();
case 4:
Undo3();
case 3:
Undo2();
case 2:
Undo1();
case 1:
printf("Failed %dn",ret);
break;
}
return ret;
8
Don't do this. The code is harder to read and doing more branches compared to simplegoto
.
– Acorn
2 days ago
4
Also, note it is wrong: ifDo5()
fails, we don't want to runUndo5()
(typically).
– Acorn
2 days ago
1
switch(ret)
should beswitch(ret-1)
. Also an (emtpy)default
case would be nice. All in all I like this approach.
– alk
2 days ago
@alk Writing the actual correct values would be better -- if you are keen on using this solution, which you should not ;) As for thedefault
case, what for?
– Acorn
2 days ago
6
The lengths people will go to avoid uncoditional jump which was basically kept in C language exactly for such cases because it makes it more readable than arrow antipattern or switch/if ladder is amazing... Take my downvote.
– Purple Ice
2 days ago
|
show 8 more comments
up vote
2
down vote
up vote
2
down vote
There are probably many ways to do this, but one idea is since you won't call one function unless the preceeding one succeeded, you could chain your function calls using else if
like this. And using a variable to track where it fails you can use a switch
statement to roll back easily too.
int ret=0;
if(Do1()) {
ret=1;
} else if(Do2()) {
ret=2;
} else if(Do3()) {
ret=3;
} else if(Do4()) {
ret=4;
} else if(Do5()) {
ret=5;
}
switch(ret) {
case 5:
Undo4();
case 4:
Undo3();
case 3:
Undo2();
case 2:
Undo1();
case 1:
printf("Failed %dn",ret);
break;
}
return ret;
There are probably many ways to do this, but one idea is since you won't call one function unless the preceeding one succeeded, you could chain your function calls using else if
like this. And using a variable to track where it fails you can use a switch
statement to roll back easily too.
int ret=0;
if(Do1()) {
ret=1;
} else if(Do2()) {
ret=2;
} else if(Do3()) {
ret=3;
} else if(Do4()) {
ret=4;
} else if(Do5()) {
ret=5;
}
switch(ret) {
case 5:
Undo4();
case 4:
Undo3();
case 3:
Undo2();
case 2:
Undo1();
case 1:
printf("Failed %dn",ret);
break;
}
return ret;
edited 2 days ago
answered 2 days ago
Chris Turner
6,2521917
6,2521917
8
Don't do this. The code is harder to read and doing more branches compared to simplegoto
.
– Acorn
2 days ago
4
Also, note it is wrong: ifDo5()
fails, we don't want to runUndo5()
(typically).
– Acorn
2 days ago
1
switch(ret)
should beswitch(ret-1)
. Also an (emtpy)default
case would be nice. All in all I like this approach.
– alk
2 days ago
@alk Writing the actual correct values would be better -- if you are keen on using this solution, which you should not ;) As for thedefault
case, what for?
– Acorn
2 days ago
6
The lengths people will go to avoid uncoditional jump which was basically kept in C language exactly for such cases because it makes it more readable than arrow antipattern or switch/if ladder is amazing... Take my downvote.
– Purple Ice
2 days ago
|
show 8 more comments
8
Don't do this. The code is harder to read and doing more branches compared to simplegoto
.
– Acorn
2 days ago
4
Also, note it is wrong: ifDo5()
fails, we don't want to runUndo5()
(typically).
– Acorn
2 days ago
1
switch(ret)
should beswitch(ret-1)
. Also an (emtpy)default
case would be nice. All in all I like this approach.
– alk
2 days ago
@alk Writing the actual correct values would be better -- if you are keen on using this solution, which you should not ;) As for thedefault
case, what for?
– Acorn
2 days ago
6
The lengths people will go to avoid uncoditional jump which was basically kept in C language exactly for such cases because it makes it more readable than arrow antipattern or switch/if ladder is amazing... Take my downvote.
– Purple Ice
2 days ago
8
8
Don't do this. The code is harder to read and doing more branches compared to simple
goto
.– Acorn
2 days ago
Don't do this. The code is harder to read and doing more branches compared to simple
goto
.– Acorn
2 days ago
4
4
Also, note it is wrong: if
Do5()
fails, we don't want to run Undo5()
(typically).– Acorn
2 days ago
Also, note it is wrong: if
Do5()
fails, we don't want to run Undo5()
(typically).– Acorn
2 days ago
1
1
switch(ret)
should be switch(ret-1)
. Also an (emtpy) default
case would be nice. All in all I like this approach.– alk
2 days ago
switch(ret)
should be switch(ret-1)
. Also an (emtpy) default
case would be nice. All in all I like this approach.– alk
2 days ago
@alk Writing the actual correct values would be better -- if you are keen on using this solution, which you should not ;) As for the
default
case, what for?– Acorn
2 days ago
@alk Writing the actual correct values would be better -- if you are keen on using this solution, which you should not ;) As for the
default
case, what for?– Acorn
2 days ago
6
6
The lengths people will go to avoid uncoditional jump which was basically kept in C language exactly for such cases because it makes it more readable than arrow antipattern or switch/if ladder is amazing... Take my downvote.
– Purple Ice
2 days ago
The lengths people will go to avoid uncoditional jump which was basically kept in C language exactly for such cases because it makes it more readable than arrow antipattern or switch/if ladder is amazing... Take my downvote.
– Purple Ice
2 days ago
|
show 8 more comments
up vote
1
down vote
Yes, as explained by other answers, using goto
for error-handling is often appropriate in C.
That said, if possible, you probably should make your cleanup code safe to call even if the corresponding action was never performed. For example, instead of:
void foo()
{
int result;
int* p = malloc(...);
if (p == NULL) { result = 1; goto err1; }
int* p2 = malloc(...);
if (p2 == NULL) { result = 2; goto err2; }
int* p3 = malloc(...);
if (p3 == NULL) { result = 3; goto err3; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Maybe we don't need p3 anymore.
free(p3);
return 0;
err3:
free(p3);
err2:
free(p2);
err1:
free(p1);
return result;
}
I'd advocate:
void foo()
{
int result = -1; // Or some generic error code for unknown errors.
int* p = NULL;
int* p2 = NULL;
int* p3 = NULL;
p = malloc(...);
if (p == NULL) { result = 1; goto exit; }
p2 = malloc(...);
if (p2 == NULL) { result = 2; goto exit; }
p3 = malloc(...);
if (p3 == NULL) { result = 3; goto exit; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Set success *only* on the successful path.
result = 0;
exit:
// free(NULL) is a no-op, so this is safe even if p3 was never allocated.
free(p3);
if (result != 0)
{
free(p2);
free(p1);
}
return result;
}
It's slightly less efficient since it requires initializing variables to NULL
, but it's more maintainable since you don't need extra labels. There's less stuff to get wrong when making changes to the code. Also, if there's cleanup code that you need on both success and failure paths, you can avoid code duplication.
add a comment |
up vote
1
down vote
Yes, as explained by other answers, using goto
for error-handling is often appropriate in C.
That said, if possible, you probably should make your cleanup code safe to call even if the corresponding action was never performed. For example, instead of:
void foo()
{
int result;
int* p = malloc(...);
if (p == NULL) { result = 1; goto err1; }
int* p2 = malloc(...);
if (p2 == NULL) { result = 2; goto err2; }
int* p3 = malloc(...);
if (p3 == NULL) { result = 3; goto err3; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Maybe we don't need p3 anymore.
free(p3);
return 0;
err3:
free(p3);
err2:
free(p2);
err1:
free(p1);
return result;
}
I'd advocate:
void foo()
{
int result = -1; // Or some generic error code for unknown errors.
int* p = NULL;
int* p2 = NULL;
int* p3 = NULL;
p = malloc(...);
if (p == NULL) { result = 1; goto exit; }
p2 = malloc(...);
if (p2 == NULL) { result = 2; goto exit; }
p3 = malloc(...);
if (p3 == NULL) { result = 3; goto exit; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Set success *only* on the successful path.
result = 0;
exit:
// free(NULL) is a no-op, so this is safe even if p3 was never allocated.
free(p3);
if (result != 0)
{
free(p2);
free(p1);
}
return result;
}
It's slightly less efficient since it requires initializing variables to NULL
, but it's more maintainable since you don't need extra labels. There's less stuff to get wrong when making changes to the code. Also, if there's cleanup code that you need on both success and failure paths, you can avoid code duplication.
add a comment |
up vote
1
down vote
up vote
1
down vote
Yes, as explained by other answers, using goto
for error-handling is often appropriate in C.
That said, if possible, you probably should make your cleanup code safe to call even if the corresponding action was never performed. For example, instead of:
void foo()
{
int result;
int* p = malloc(...);
if (p == NULL) { result = 1; goto err1; }
int* p2 = malloc(...);
if (p2 == NULL) { result = 2; goto err2; }
int* p3 = malloc(...);
if (p3 == NULL) { result = 3; goto err3; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Maybe we don't need p3 anymore.
free(p3);
return 0;
err3:
free(p3);
err2:
free(p2);
err1:
free(p1);
return result;
}
I'd advocate:
void foo()
{
int result = -1; // Or some generic error code for unknown errors.
int* p = NULL;
int* p2 = NULL;
int* p3 = NULL;
p = malloc(...);
if (p == NULL) { result = 1; goto exit; }
p2 = malloc(...);
if (p2 == NULL) { result = 2; goto exit; }
p3 = malloc(...);
if (p3 == NULL) { result = 3; goto exit; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Set success *only* on the successful path.
result = 0;
exit:
// free(NULL) is a no-op, so this is safe even if p3 was never allocated.
free(p3);
if (result != 0)
{
free(p2);
free(p1);
}
return result;
}
It's slightly less efficient since it requires initializing variables to NULL
, but it's more maintainable since you don't need extra labels. There's less stuff to get wrong when making changes to the code. Also, if there's cleanup code that you need on both success and failure paths, you can avoid code duplication.
Yes, as explained by other answers, using goto
for error-handling is often appropriate in C.
That said, if possible, you probably should make your cleanup code safe to call even if the corresponding action was never performed. For example, instead of:
void foo()
{
int result;
int* p = malloc(...);
if (p == NULL) { result = 1; goto err1; }
int* p2 = malloc(...);
if (p2 == NULL) { result = 2; goto err2; }
int* p3 = malloc(...);
if (p3 == NULL) { result = 3; goto err3; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Maybe we don't need p3 anymore.
free(p3);
return 0;
err3:
free(p3);
err2:
free(p2);
err1:
free(p1);
return result;
}
I'd advocate:
void foo()
{
int result = -1; // Or some generic error code for unknown errors.
int* p = NULL;
int* p2 = NULL;
int* p3 = NULL;
p = malloc(...);
if (p == NULL) { result = 1; goto exit; }
p2 = malloc(...);
if (p2 == NULL) { result = 2; goto exit; }
p3 = malloc(...);
if (p3 == NULL) { result = 3; goto exit; }
// Do something with p, p2, and p3.
bar(p, p2, p3);
// Set success *only* on the successful path.
result = 0;
exit:
// free(NULL) is a no-op, so this is safe even if p3 was never allocated.
free(p3);
if (result != 0)
{
free(p2);
free(p1);
}
return result;
}
It's slightly less efficient since it requires initializing variables to NULL
, but it's more maintainable since you don't need extra labels. There's less stuff to get wrong when making changes to the code. Also, if there's cleanup code that you need on both success and failure paths, you can avoid code duplication.
answered 2 days ago
jamesdlin
25.8k65791
25.8k65791
add a comment |
add a comment |
up vote
0
down vote
I typically approach this kind of problem by nesting the conditionals:
int rval = 1;
if (!Do1()) {
if (!Do2()) {
if (!Do3()) {
if (!Do4()) {
if (!Do5()) {
return 0;
// or "goto succeeded", or ...;
} else {
printf("Failed 5");
rval = 5;
}
Undo4();
} else {
printf("Failed 4");
rval = 4;
}
Undo3();
} else {
printf("Failed 3");
rval = 3;
}
Undo2();
} else {
printf("Failed 2");
rval = 2;
}
Undo1();
} else {
printf("Failed 1");
rval = 1;
}
return rval;
Usually, for me, the DoX()
are some kind of resource acquisition, such as malloc()
, and the UndoX()
are corresponding resource releases that should be performed only in the event of failure. The nesting clearly shows the association between corresponding acquisitions and releases, and avoids the need for repetition of the code for undo operations. It's also very easy to write -- you don't need to create or maintain labels, and it's easy to put the resource release in the right place as soon as you write the acquisition.
This approach does sometimes produce deeply nested code. That doesn't bother me much, but you might consider it an issue.
2
'deeply nested' - plus error and error handling get separated by quite some distance, the more for the less deeply nested ones...
– Aconcagua
yesterday
@Aconcagua, separation of error and error-handling is a pretty consistent characteristic of all of these approaches. It follows directly from avoiding repetition of the error-handling code.
– John Bollinger
yesterday
add a comment |
up vote
0
down vote
I typically approach this kind of problem by nesting the conditionals:
int rval = 1;
if (!Do1()) {
if (!Do2()) {
if (!Do3()) {
if (!Do4()) {
if (!Do5()) {
return 0;
// or "goto succeeded", or ...;
} else {
printf("Failed 5");
rval = 5;
}
Undo4();
} else {
printf("Failed 4");
rval = 4;
}
Undo3();
} else {
printf("Failed 3");
rval = 3;
}
Undo2();
} else {
printf("Failed 2");
rval = 2;
}
Undo1();
} else {
printf("Failed 1");
rval = 1;
}
return rval;
Usually, for me, the DoX()
are some kind of resource acquisition, such as malloc()
, and the UndoX()
are corresponding resource releases that should be performed only in the event of failure. The nesting clearly shows the association between corresponding acquisitions and releases, and avoids the need for repetition of the code for undo operations. It's also very easy to write -- you don't need to create or maintain labels, and it's easy to put the resource release in the right place as soon as you write the acquisition.
This approach does sometimes produce deeply nested code. That doesn't bother me much, but you might consider it an issue.
2
'deeply nested' - plus error and error handling get separated by quite some distance, the more for the less deeply nested ones...
– Aconcagua
yesterday
@Aconcagua, separation of error and error-handling is a pretty consistent characteristic of all of these approaches. It follows directly from avoiding repetition of the error-handling code.
– John Bollinger
yesterday
add a comment |
up vote
0
down vote
up vote
0
down vote
I typically approach this kind of problem by nesting the conditionals:
int rval = 1;
if (!Do1()) {
if (!Do2()) {
if (!Do3()) {
if (!Do4()) {
if (!Do5()) {
return 0;
// or "goto succeeded", or ...;
} else {
printf("Failed 5");
rval = 5;
}
Undo4();
} else {
printf("Failed 4");
rval = 4;
}
Undo3();
} else {
printf("Failed 3");
rval = 3;
}
Undo2();
} else {
printf("Failed 2");
rval = 2;
}
Undo1();
} else {
printf("Failed 1");
rval = 1;
}
return rval;
Usually, for me, the DoX()
are some kind of resource acquisition, such as malloc()
, and the UndoX()
are corresponding resource releases that should be performed only in the event of failure. The nesting clearly shows the association between corresponding acquisitions and releases, and avoids the need for repetition of the code for undo operations. It's also very easy to write -- you don't need to create or maintain labels, and it's easy to put the resource release in the right place as soon as you write the acquisition.
This approach does sometimes produce deeply nested code. That doesn't bother me much, but you might consider it an issue.
I typically approach this kind of problem by nesting the conditionals:
int rval = 1;
if (!Do1()) {
if (!Do2()) {
if (!Do3()) {
if (!Do4()) {
if (!Do5()) {
return 0;
// or "goto succeeded", or ...;
} else {
printf("Failed 5");
rval = 5;
}
Undo4();
} else {
printf("Failed 4");
rval = 4;
}
Undo3();
} else {
printf("Failed 3");
rval = 3;
}
Undo2();
} else {
printf("Failed 2");
rval = 2;
}
Undo1();
} else {
printf("Failed 1");
rval = 1;
}
return rval;
Usually, for me, the DoX()
are some kind of resource acquisition, such as malloc()
, and the UndoX()
are corresponding resource releases that should be performed only in the event of failure. The nesting clearly shows the association between corresponding acquisitions and releases, and avoids the need for repetition of the code for undo operations. It's also very easy to write -- you don't need to create or maintain labels, and it's easy to put the resource release in the right place as soon as you write the acquisition.
This approach does sometimes produce deeply nested code. That doesn't bother me much, but you might consider it an issue.
answered 2 days ago
John Bollinger
76.6k63771
76.6k63771
2
'deeply nested' - plus error and error handling get separated by quite some distance, the more for the less deeply nested ones...
– Aconcagua
yesterday
@Aconcagua, separation of error and error-handling is a pretty consistent characteristic of all of these approaches. It follows directly from avoiding repetition of the error-handling code.
– John Bollinger
yesterday
add a comment |
2
'deeply nested' - plus error and error handling get separated by quite some distance, the more for the less deeply nested ones...
– Aconcagua
yesterday
@Aconcagua, separation of error and error-handling is a pretty consistent characteristic of all of these approaches. It follows directly from avoiding repetition of the error-handling code.
– John Bollinger
yesterday
2
2
'deeply nested' - plus error and error handling get separated by quite some distance, the more for the less deeply nested ones...
– Aconcagua
yesterday
'deeply nested' - plus error and error handling get separated by quite some distance, the more for the less deeply nested ones...
– Aconcagua
yesterday
@Aconcagua, separation of error and error-handling is a pretty consistent characteristic of all of these approaches. It follows directly from avoiding repetition of the error-handling code.
– John Bollinger
yesterday
@Aconcagua, separation of error and error-handling is a pretty consistent characteristic of all of these approaches. It follows directly from avoiding repetition of the error-handling code.
– John Bollinger
yesterday
add a comment |
up vote
0
down vote
Here is an answer that I have found resilient to bugs.
Yes. It uses goto
. I firmly believe you should use what gives you most clarity, rather than just blindly following the advice of those before you (goto
as a construct can make spaghetti code, but in this instance every other error handling method ususally ends up more spaghetti-like than using this method of goto
, so IMO it's superior).
Some people may not like the form of this code, but I contest that when used to the style it is cleaner, easier to read (when everything's lined up, of course), and much more resilient to errors. If you have the properly linter/static analysis setup, and you're working with POSIX, it pretty much requires you to code in this fashion to allow for good error handling.
static char *readbuf(char *path)
{
struct stat st;
char *s = NULL;
size_t size = 0;
int fd = -1;
if (!path) { return NULL; }
if ((stat(path, &st)) < 0) { perror(path); goto _throw; }
size = st.st_size;
if (size == 0) { printf("%s is empty!n", path); goto _throw; }
if (!(s = calloc(size, 1))) { perror("calloc"); goto _throw; }
fd = open(path, O_RDONLY);
if (fd < -1) { perror(path); goto _throw; }
if ((read(fd, s, size)) < 0) { perror("read"); goto _throw; }
close(fd); /* There's really no point checking close for errors */
return s;
_throw:
if (fd > 0) close(fd);
if (s) free(s);
return NULL;
}
add a comment |
up vote
0
down vote
Here is an answer that I have found resilient to bugs.
Yes. It uses goto
. I firmly believe you should use what gives you most clarity, rather than just blindly following the advice of those before you (goto
as a construct can make spaghetti code, but in this instance every other error handling method ususally ends up more spaghetti-like than using this method of goto
, so IMO it's superior).
Some people may not like the form of this code, but I contest that when used to the style it is cleaner, easier to read (when everything's lined up, of course), and much more resilient to errors. If you have the properly linter/static analysis setup, and you're working with POSIX, it pretty much requires you to code in this fashion to allow for good error handling.
static char *readbuf(char *path)
{
struct stat st;
char *s = NULL;
size_t size = 0;
int fd = -1;
if (!path) { return NULL; }
if ((stat(path, &st)) < 0) { perror(path); goto _throw; }
size = st.st_size;
if (size == 0) { printf("%s is empty!n", path); goto _throw; }
if (!(s = calloc(size, 1))) { perror("calloc"); goto _throw; }
fd = open(path, O_RDONLY);
if (fd < -1) { perror(path); goto _throw; }
if ((read(fd, s, size)) < 0) { perror("read"); goto _throw; }
close(fd); /* There's really no point checking close for errors */
return s;
_throw:
if (fd > 0) close(fd);
if (s) free(s);
return NULL;
}
add a comment |
up vote
0
down vote
up vote
0
down vote
Here is an answer that I have found resilient to bugs.
Yes. It uses goto
. I firmly believe you should use what gives you most clarity, rather than just blindly following the advice of those before you (goto
as a construct can make spaghetti code, but in this instance every other error handling method ususally ends up more spaghetti-like than using this method of goto
, so IMO it's superior).
Some people may not like the form of this code, but I contest that when used to the style it is cleaner, easier to read (when everything's lined up, of course), and much more resilient to errors. If you have the properly linter/static analysis setup, and you're working with POSIX, it pretty much requires you to code in this fashion to allow for good error handling.
static char *readbuf(char *path)
{
struct stat st;
char *s = NULL;
size_t size = 0;
int fd = -1;
if (!path) { return NULL; }
if ((stat(path, &st)) < 0) { perror(path); goto _throw; }
size = st.st_size;
if (size == 0) { printf("%s is empty!n", path); goto _throw; }
if (!(s = calloc(size, 1))) { perror("calloc"); goto _throw; }
fd = open(path, O_RDONLY);
if (fd < -1) { perror(path); goto _throw; }
if ((read(fd, s, size)) < 0) { perror("read"); goto _throw; }
close(fd); /* There's really no point checking close for errors */
return s;
_throw:
if (fd > 0) close(fd);
if (s) free(s);
return NULL;
}
Here is an answer that I have found resilient to bugs.
Yes. It uses goto
. I firmly believe you should use what gives you most clarity, rather than just blindly following the advice of those before you (goto
as a construct can make spaghetti code, but in this instance every other error handling method ususally ends up more spaghetti-like than using this method of goto
, so IMO it's superior).
Some people may not like the form of this code, but I contest that when used to the style it is cleaner, easier to read (when everything's lined up, of course), and much more resilient to errors. If you have the properly linter/static analysis setup, and you're working with POSIX, it pretty much requires you to code in this fashion to allow for good error handling.
static char *readbuf(char *path)
{
struct stat st;
char *s = NULL;
size_t size = 0;
int fd = -1;
if (!path) { return NULL; }
if ((stat(path, &st)) < 0) { perror(path); goto _throw; }
size = st.st_size;
if (size == 0) { printf("%s is empty!n", path); goto _throw; }
if (!(s = calloc(size, 1))) { perror("calloc"); goto _throw; }
fd = open(path, O_RDONLY);
if (fd < -1) { perror(path); goto _throw; }
if ((read(fd, s, size)) < 0) { perror("read"); goto _throw; }
close(fd); /* There's really no point checking close for errors */
return s;
_throw:
if (fd > 0) close(fd);
if (s) free(s);
return NULL;
}
answered yesterday
Finn O'leary
13912
13912
add a comment |
add a comment |
up vote
-1
down vote
If the functions return some kind of state pointer or handle (like most allocation & initialization functions would), you can quite cleanly do this without goto
by giving initial values to variables. Then you can have a single deallocation function that can handle the case where only part of the resources has been allocated.
For example:
void *mymemoryblock = NULL;
FILE *myfile = NULL;
int mysocket = -1;
bool allocate_everything()
{
mymemoryblock = malloc(1000);
if (!mymemoryblock)
{
return false;
}
myfile = fopen("/file", "r");
if (!myfile)
{
return false;
}
mysocket = socket(AF_INET, SOCK_STREAM, 0);
if (mysocket < 0)
{
return false;
}
return true;
}
void deallocate_everything()
{
if (mysocket >= 0)
{
close(mysocket);
mysocket = -1;
}
if (myfile)
{
fclose(myfile);
myfile = NULL;
}
if (mymemoryblock)
{
free(mymemoryblock);
mymemoryblock = NULL;
}
}
And then just do:
if (allocate_everything())
{
do_the_deed();
}
deallocate_everything();
"If the functions return some kind of state pointer or handle..." Yes, but it is not the general case. Further, your solution requires 3 function for each function that allocates resources, plus global variables (or passing things around).
– Acorn
2 days ago
add a comment |
up vote
-1
down vote
If the functions return some kind of state pointer or handle (like most allocation & initialization functions would), you can quite cleanly do this without goto
by giving initial values to variables. Then you can have a single deallocation function that can handle the case where only part of the resources has been allocated.
For example:
void *mymemoryblock = NULL;
FILE *myfile = NULL;
int mysocket = -1;
bool allocate_everything()
{
mymemoryblock = malloc(1000);
if (!mymemoryblock)
{
return false;
}
myfile = fopen("/file", "r");
if (!myfile)
{
return false;
}
mysocket = socket(AF_INET, SOCK_STREAM, 0);
if (mysocket < 0)
{
return false;
}
return true;
}
void deallocate_everything()
{
if (mysocket >= 0)
{
close(mysocket);
mysocket = -1;
}
if (myfile)
{
fclose(myfile);
myfile = NULL;
}
if (mymemoryblock)
{
free(mymemoryblock);
mymemoryblock = NULL;
}
}
And then just do:
if (allocate_everything())
{
do_the_deed();
}
deallocate_everything();
"If the functions return some kind of state pointer or handle..." Yes, but it is not the general case. Further, your solution requires 3 function for each function that allocates resources, plus global variables (or passing things around).
– Acorn
2 days ago
add a comment |
up vote
-1
down vote
up vote
-1
down vote
If the functions return some kind of state pointer or handle (like most allocation & initialization functions would), you can quite cleanly do this without goto
by giving initial values to variables. Then you can have a single deallocation function that can handle the case where only part of the resources has been allocated.
For example:
void *mymemoryblock = NULL;
FILE *myfile = NULL;
int mysocket = -1;
bool allocate_everything()
{
mymemoryblock = malloc(1000);
if (!mymemoryblock)
{
return false;
}
myfile = fopen("/file", "r");
if (!myfile)
{
return false;
}
mysocket = socket(AF_INET, SOCK_STREAM, 0);
if (mysocket < 0)
{
return false;
}
return true;
}
void deallocate_everything()
{
if (mysocket >= 0)
{
close(mysocket);
mysocket = -1;
}
if (myfile)
{
fclose(myfile);
myfile = NULL;
}
if (mymemoryblock)
{
free(mymemoryblock);
mymemoryblock = NULL;
}
}
And then just do:
if (allocate_everything())
{
do_the_deed();
}
deallocate_everything();
If the functions return some kind of state pointer or handle (like most allocation & initialization functions would), you can quite cleanly do this without goto
by giving initial values to variables. Then you can have a single deallocation function that can handle the case where only part of the resources has been allocated.
For example:
void *mymemoryblock = NULL;
FILE *myfile = NULL;
int mysocket = -1;
bool allocate_everything()
{
mymemoryblock = malloc(1000);
if (!mymemoryblock)
{
return false;
}
myfile = fopen("/file", "r");
if (!myfile)
{
return false;
}
mysocket = socket(AF_INET, SOCK_STREAM, 0);
if (mysocket < 0)
{
return false;
}
return true;
}
void deallocate_everything()
{
if (mysocket >= 0)
{
close(mysocket);
mysocket = -1;
}
if (myfile)
{
fclose(myfile);
myfile = NULL;
}
if (mymemoryblock)
{
free(mymemoryblock);
mymemoryblock = NULL;
}
}
And then just do:
if (allocate_everything())
{
do_the_deed();
}
deallocate_everything();
answered 2 days ago
jpa
5,1391226
5,1391226
"If the functions return some kind of state pointer or handle..." Yes, but it is not the general case. Further, your solution requires 3 function for each function that allocates resources, plus global variables (or passing things around).
– Acorn
2 days ago
add a comment |
"If the functions return some kind of state pointer or handle..." Yes, but it is not the general case. Further, your solution requires 3 function for each function that allocates resources, plus global variables (or passing things around).
– Acorn
2 days ago
"If the functions return some kind of state pointer or handle..." Yes, but it is not the general case. Further, your solution requires 3 function for each function that allocates resources, plus global variables (or passing things around).
– Acorn
2 days ago
"If the functions return some kind of state pointer or handle..." Yes, but it is not the general case. Further, your solution requires 3 function for each function that allocates resources, plus global variables (or passing things around).
– Acorn
2 days ago
add a comment |
up vote
-1
down vote
TL;DR:
I believe it should be written as:
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Detailed explanation:
If nothing can be assumed what-so-ever about the function types, we can't easily use an array of function pointers, which would otherwise be the correct answer.
Assuming all function types are incompatible, then we would have to wrap in the original obscure design containing all those non-compatible functions, inside something else.
We should make something that is readable, maintainable, fast. We should avoid tight coupling, so that the undo behavior of "Do_x" doesn't depend on the undo behavior of "Do_y".
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Where do_func
is the function doing all the calls required by the algorithm, and the printf
is the UI output, separated from the algorithm logic.
do_func
would be implemented like a wrapper function around the actual function calls, handling the outcome depending on the result:
(With gcc -O3, do_func
is inlined in the caller, so there is no overhead for having 2 separate functions)
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
Here the specific behavior is controlled by the array undo
, which is a wrapper around the various non-compatible functions. Which functions to to call, in which order, is all part of the specific behavior tied to each result code.
We need to tidy it all up, so that we can couple a certain behavior to a certain result code. Then when needed, we only change the code in one single place if the behavior should be changed during maintenance:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
MCVE:
#include <stdbool.h>
#include <stdio.h>
// some nonsense functions:
bool Do1 (void) { puts(__func__); return false; }
bool Do2 (void) { puts(__func__); return false; }
bool Do3 (void) { puts(__func__); return false; }
bool Do4 (void) { puts(__func__); return false; }
bool Do5 (void) { puts(__func__); return true; }
void Undo1 (void) { puts(__func__); }
void Undo2 (void) { puts(__func__); }
void Undo3 (void) { puts(__func__); }
void Undo4 (void) { puts(__func__); }
void Undo5 (void) { puts(__func__); }
// wrappers specifying undo behavior:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Output:
Do1
Do2
Do3
Do4
Do5
Undo4
Undo3
Undo2
Undo1
Failed 5
12
So, for each single function in your code that allocates resources, you are going to write 4 functions instead (which, some of them duplicateUndoN()
calls in turn). Plus a wrapper. Plus a type. Plus a global array of that type. No further comments.
– Acorn
2 days ago
@Acorn No, for every ADT in my code, I'm going to write a user-friend API, a clarity of who's responsible for allocation/deallocation, private encapsulation, modular, maintainable code, without tight coupling, where everything is autonomous. In this particular case, it looks like we are patching up some old, broken design, in which case you need wrappers - which is the fault of the original (lack of) design. Of course you don't need that level of abstraction if it's just some quick & dirty hack, but in that case all that is best practices is thrown out the window anyway.
– Lundin
14 hours ago
add a comment |
up vote
-1
down vote
TL;DR:
I believe it should be written as:
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Detailed explanation:
If nothing can be assumed what-so-ever about the function types, we can't easily use an array of function pointers, which would otherwise be the correct answer.
Assuming all function types are incompatible, then we would have to wrap in the original obscure design containing all those non-compatible functions, inside something else.
We should make something that is readable, maintainable, fast. We should avoid tight coupling, so that the undo behavior of "Do_x" doesn't depend on the undo behavior of "Do_y".
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Where do_func
is the function doing all the calls required by the algorithm, and the printf
is the UI output, separated from the algorithm logic.
do_func
would be implemented like a wrapper function around the actual function calls, handling the outcome depending on the result:
(With gcc -O3, do_func
is inlined in the caller, so there is no overhead for having 2 separate functions)
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
Here the specific behavior is controlled by the array undo
, which is a wrapper around the various non-compatible functions. Which functions to to call, in which order, is all part of the specific behavior tied to each result code.
We need to tidy it all up, so that we can couple a certain behavior to a certain result code. Then when needed, we only change the code in one single place if the behavior should be changed during maintenance:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
MCVE:
#include <stdbool.h>
#include <stdio.h>
// some nonsense functions:
bool Do1 (void) { puts(__func__); return false; }
bool Do2 (void) { puts(__func__); return false; }
bool Do3 (void) { puts(__func__); return false; }
bool Do4 (void) { puts(__func__); return false; }
bool Do5 (void) { puts(__func__); return true; }
void Undo1 (void) { puts(__func__); }
void Undo2 (void) { puts(__func__); }
void Undo3 (void) { puts(__func__); }
void Undo4 (void) { puts(__func__); }
void Undo5 (void) { puts(__func__); }
// wrappers specifying undo behavior:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Output:
Do1
Do2
Do3
Do4
Do5
Undo4
Undo3
Undo2
Undo1
Failed 5
12
So, for each single function in your code that allocates resources, you are going to write 4 functions instead (which, some of them duplicateUndoN()
calls in turn). Plus a wrapper. Plus a type. Plus a global array of that type. No further comments.
– Acorn
2 days ago
@Acorn No, for every ADT in my code, I'm going to write a user-friend API, a clarity of who's responsible for allocation/deallocation, private encapsulation, modular, maintainable code, without tight coupling, where everything is autonomous. In this particular case, it looks like we are patching up some old, broken design, in which case you need wrappers - which is the fault of the original (lack of) design. Of course you don't need that level of abstraction if it's just some quick & dirty hack, but in that case all that is best practices is thrown out the window anyway.
– Lundin
14 hours ago
add a comment |
up vote
-1
down vote
up vote
-1
down vote
TL;DR:
I believe it should be written as:
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Detailed explanation:
If nothing can be assumed what-so-ever about the function types, we can't easily use an array of function pointers, which would otherwise be the correct answer.
Assuming all function types are incompatible, then we would have to wrap in the original obscure design containing all those non-compatible functions, inside something else.
We should make something that is readable, maintainable, fast. We should avoid tight coupling, so that the undo behavior of "Do_x" doesn't depend on the undo behavior of "Do_y".
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Where do_func
is the function doing all the calls required by the algorithm, and the printf
is the UI output, separated from the algorithm logic.
do_func
would be implemented like a wrapper function around the actual function calls, handling the outcome depending on the result:
(With gcc -O3, do_func
is inlined in the caller, so there is no overhead for having 2 separate functions)
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
Here the specific behavior is controlled by the array undo
, which is a wrapper around the various non-compatible functions. Which functions to to call, in which order, is all part of the specific behavior tied to each result code.
We need to tidy it all up, so that we can couple a certain behavior to a certain result code. Then when needed, we only change the code in one single place if the behavior should be changed during maintenance:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
MCVE:
#include <stdbool.h>
#include <stdio.h>
// some nonsense functions:
bool Do1 (void) { puts(__func__); return false; }
bool Do2 (void) { puts(__func__); return false; }
bool Do3 (void) { puts(__func__); return false; }
bool Do4 (void) { puts(__func__); return false; }
bool Do5 (void) { puts(__func__); return true; }
void Undo1 (void) { puts(__func__); }
void Undo2 (void) { puts(__func__); }
void Undo3 (void) { puts(__func__); }
void Undo4 (void) { puts(__func__); }
void Undo5 (void) { puts(__func__); }
// wrappers specifying undo behavior:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Output:
Do1
Do2
Do3
Do4
Do5
Undo4
Undo3
Undo2
Undo1
Failed 5
TL;DR:
I believe it should be written as:
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Detailed explanation:
If nothing can be assumed what-so-ever about the function types, we can't easily use an array of function pointers, which would otherwise be the correct answer.
Assuming all function types are incompatible, then we would have to wrap in the original obscure design containing all those non-compatible functions, inside something else.
We should make something that is readable, maintainable, fast. We should avoid tight coupling, so that the undo behavior of "Do_x" doesn't depend on the undo behavior of "Do_y".
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Where do_func
is the function doing all the calls required by the algorithm, and the printf
is the UI output, separated from the algorithm logic.
do_func
would be implemented like a wrapper function around the actual function calls, handling the outcome depending on the result:
(With gcc -O3, do_func
is inlined in the caller, so there is no overhead for having 2 separate functions)
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
Here the specific behavior is controlled by the array undo
, which is a wrapper around the various non-compatible functions. Which functions to to call, in which order, is all part of the specific behavior tied to each result code.
We need to tidy it all up, so that we can couple a certain behavior to a certain result code. Then when needed, we only change the code in one single place if the behavior should be changed during maintenance:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
MCVE:
#include <stdbool.h>
#include <stdio.h>
// some nonsense functions:
bool Do1 (void) { puts(__func__); return false; }
bool Do2 (void) { puts(__func__); return false; }
bool Do3 (void) { puts(__func__); return false; }
bool Do4 (void) { puts(__func__); return false; }
bool Do5 (void) { puts(__func__); return true; }
void Undo1 (void) { puts(__func__); }
void Undo2 (void) { puts(__func__); }
void Undo3 (void) { puts(__func__); }
void Undo4 (void) { puts(__func__); }
void Undo5 (void) { puts(__func__); }
// wrappers specifying undo behavior:
void Undo_stuff1 (void) { }
void Undo_stuff2 (void) { Undo1(); }
void Undo_stuff3 (void) { Undo2(); Undo1(); }
void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }
void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }
typedef void Undo_stuff_t (void);
static Undo_stuff_t* undo[5] =
{
Undo_stuff1,
Undo_stuff2,
Undo_stuff3,
Undo_stuff4,
Undo_stuff5,
};
int do_it (void)
{
if(Do1()) { return 1; };
if(Do2()) { return 2; };
if(Do3()) { return 3; };
if(Do4()) { return 4; };
if(Do5()) { return 5; };
return 0;
}
int do_func (void)
{
int result = do_it();
if(result != 0)
{
undo[result-1]();
}
return result;
}
int main (void)
{
int result = do_func();
printf("Failed %dn", result);
}
Output:
Do1
Do2
Do3
Do4
Do5
Undo4
Undo3
Undo2
Undo1
Failed 5
edited 2 days ago
answered 2 days ago
Lundin
105k16153257
105k16153257
12
So, for each single function in your code that allocates resources, you are going to write 4 functions instead (which, some of them duplicateUndoN()
calls in turn). Plus a wrapper. Plus a type. Plus a global array of that type. No further comments.
– Acorn
2 days ago
@Acorn No, for every ADT in my code, I'm going to write a user-friend API, a clarity of who's responsible for allocation/deallocation, private encapsulation, modular, maintainable code, without tight coupling, where everything is autonomous. In this particular case, it looks like we are patching up some old, broken design, in which case you need wrappers - which is the fault of the original (lack of) design. Of course you don't need that level of abstraction if it's just some quick & dirty hack, but in that case all that is best practices is thrown out the window anyway.
– Lundin
14 hours ago
add a comment |
12
So, for each single function in your code that allocates resources, you are going to write 4 functions instead (which, some of them duplicateUndoN()
calls in turn). Plus a wrapper. Plus a type. Plus a global array of that type. No further comments.
– Acorn
2 days ago
@Acorn No, for every ADT in my code, I'm going to write a user-friend API, a clarity of who's responsible for allocation/deallocation, private encapsulation, modular, maintainable code, without tight coupling, where everything is autonomous. In this particular case, it looks like we are patching up some old, broken design, in which case you need wrappers - which is the fault of the original (lack of) design. Of course you don't need that level of abstraction if it's just some quick & dirty hack, but in that case all that is best practices is thrown out the window anyway.
– Lundin
14 hours ago
12
12
So, for each single function in your code that allocates resources, you are going to write 4 functions instead (which, some of them duplicate
UndoN()
calls in turn). Plus a wrapper. Plus a type. Plus a global array of that type. No further comments.– Acorn
2 days ago
So, for each single function in your code that allocates resources, you are going to write 4 functions instead (which, some of them duplicate
UndoN()
calls in turn). Plus a wrapper. Plus a type. Plus a global array of that type. No further comments.– Acorn
2 days ago
@Acorn No, for every ADT in my code, I'm going to write a user-friend API, a clarity of who's responsible for allocation/deallocation, private encapsulation, modular, maintainable code, without tight coupling, where everything is autonomous. In this particular case, it looks like we are patching up some old, broken design, in which case you need wrappers - which is the fault of the original (lack of) design. Of course you don't need that level of abstraction if it's just some quick & dirty hack, but in that case all that is best practices is thrown out the window anyway.
– Lundin
14 hours ago
@Acorn No, for every ADT in my code, I'm going to write a user-friend API, a clarity of who's responsible for allocation/deallocation, private encapsulation, modular, maintainable code, without tight coupling, where everything is autonomous. In this particular case, it looks like we are patching up some old, broken design, in which case you need wrappers - which is the fault of the original (lack of) design. Of course you don't need that level of abstraction if it's just some quick & dirty hack, but in that case all that is best practices is thrown out the window anyway.
– Lundin
14 hours ago
add a comment |
up vote
-2
down vote
typedef void(*undoer)();
int undo( undoer*const* list ) {
while(*list) {
(*list)();
++list;
}
}
void undo_push( undoer** list, undoer* undo ) {
if (!undo) return;
// swap
undoer* tmp = *list;
*list = undo;
undo = tmp;
undo_push( list+1, undo );
}
int func() {
undoer undoers[6]={0};
if (Do1()) { printf("Failed 1"); return 1; }
undo_push( undoers, Undo1 );
if (Do2()) { undo(undoers); printf("Failed 2"); return 2; }
undo_push( undoers, Undo2 );
if (Do3()) { undo(undoers); printf("Failed 3"); return 3; }
undo_push( undoers, Undo3 );
if (Do4()) { undo(undoers); printf("Failed 4"); return 4; }
undo_push( undoers, Undo4 );
if (Do5()) { undo(undoers); printf("Failed 5"); return 5; }
return 6;
}
I made undo_push
do the O(n) work. This is less efficient than having undo
do the O(n) work, as we expect more push's than undos. But this version was a touch simpler.
A more industrial strength version would have head and tail pointers and even capacity.
The basic idea is to keep a queue of undo actions in a stack, then execute them if you need to clean up.
Everything is local here, so we don't pollute global state.
struct undoer {
void(*action)(void*);
void(*cleanup)(void*);
void* state;
};
struct undoers {
undoer* top;
undoer buff[5];
};
void undo( undoers u ) {
while (u.top != buff)
{
(u.top->action)(u.top->state);
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pundo(void* pu) {
undo( *(undoers*)pu );
free(pu);
}
void cleanup_undoers(undoers u) {
while (u.top != buff)
{
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pcleanup_undoers(void* pu) {
cleanup_undoers(*(undoers*)pu);
free(pu);
}
void push_undoer( undoers* to_here, undoer u ) {
if (to_here->top != (to_here->buff+5))
{
to_here->top = u;
++(to_here->top)
return;
}
undoers* chain = (undoers*)malloc( sizeof(undoers) );
memcpy(chain, to_here, sizeof(undoers));
memset(to_here, 0, sizeof(undoers));
undoer chainer;
chainer.action = pundo;
chainer.cleanup = pcleanup_undoers;
chainer.data = chain;
push_undoer( to_here, chainer );
push_undoer( to_here, u );
}
void paction( void* p ) {
(void)(*a)() = ((void)(*)());
a();
}
void push_undo( undoers* to_here, void(*action)() ) {
undor u;
u.action = paction;
u.cleanup = 0;
u.data = (void*)action;
push_undoer(to_here, u);
}
then you get:
int func() {
undoers u={0};
if (Do1()) { printf("Failed 1"); return 1; }
push_undo( &u, Undo1 );
if (Do2()) { undo(u); printf("Failed 2"); return 2; }
push_undo( &u, Undo2 );
if (Do3()) { undo(u); printf("Failed 3"); return 3; }
push_undo( &u, Undo3 );
if (Do4()) { undo(u); printf("Failed 4"); return 4; }
push_undo( &u, Undo4 );
if (Do5()) { undo(u); printf("Failed 5"); return 5; }
cleanup_undoers(u);
return 6;
}
but that is getting ridiculous.
1
More complex, requires thatDoN
/UndoN
are actual functions (and the same signature), requires stack space, slower.
– Acorn
2 days ago
add a comment |
up vote
-2
down vote
typedef void(*undoer)();
int undo( undoer*const* list ) {
while(*list) {
(*list)();
++list;
}
}
void undo_push( undoer** list, undoer* undo ) {
if (!undo) return;
// swap
undoer* tmp = *list;
*list = undo;
undo = tmp;
undo_push( list+1, undo );
}
int func() {
undoer undoers[6]={0};
if (Do1()) { printf("Failed 1"); return 1; }
undo_push( undoers, Undo1 );
if (Do2()) { undo(undoers); printf("Failed 2"); return 2; }
undo_push( undoers, Undo2 );
if (Do3()) { undo(undoers); printf("Failed 3"); return 3; }
undo_push( undoers, Undo3 );
if (Do4()) { undo(undoers); printf("Failed 4"); return 4; }
undo_push( undoers, Undo4 );
if (Do5()) { undo(undoers); printf("Failed 5"); return 5; }
return 6;
}
I made undo_push
do the O(n) work. This is less efficient than having undo
do the O(n) work, as we expect more push's than undos. But this version was a touch simpler.
A more industrial strength version would have head and tail pointers and even capacity.
The basic idea is to keep a queue of undo actions in a stack, then execute them if you need to clean up.
Everything is local here, so we don't pollute global state.
struct undoer {
void(*action)(void*);
void(*cleanup)(void*);
void* state;
};
struct undoers {
undoer* top;
undoer buff[5];
};
void undo( undoers u ) {
while (u.top != buff)
{
(u.top->action)(u.top->state);
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pundo(void* pu) {
undo( *(undoers*)pu );
free(pu);
}
void cleanup_undoers(undoers u) {
while (u.top != buff)
{
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pcleanup_undoers(void* pu) {
cleanup_undoers(*(undoers*)pu);
free(pu);
}
void push_undoer( undoers* to_here, undoer u ) {
if (to_here->top != (to_here->buff+5))
{
to_here->top = u;
++(to_here->top)
return;
}
undoers* chain = (undoers*)malloc( sizeof(undoers) );
memcpy(chain, to_here, sizeof(undoers));
memset(to_here, 0, sizeof(undoers));
undoer chainer;
chainer.action = pundo;
chainer.cleanup = pcleanup_undoers;
chainer.data = chain;
push_undoer( to_here, chainer );
push_undoer( to_here, u );
}
void paction( void* p ) {
(void)(*a)() = ((void)(*)());
a();
}
void push_undo( undoers* to_here, void(*action)() ) {
undor u;
u.action = paction;
u.cleanup = 0;
u.data = (void*)action;
push_undoer(to_here, u);
}
then you get:
int func() {
undoers u={0};
if (Do1()) { printf("Failed 1"); return 1; }
push_undo( &u, Undo1 );
if (Do2()) { undo(u); printf("Failed 2"); return 2; }
push_undo( &u, Undo2 );
if (Do3()) { undo(u); printf("Failed 3"); return 3; }
push_undo( &u, Undo3 );
if (Do4()) { undo(u); printf("Failed 4"); return 4; }
push_undo( &u, Undo4 );
if (Do5()) { undo(u); printf("Failed 5"); return 5; }
cleanup_undoers(u);
return 6;
}
but that is getting ridiculous.
1
More complex, requires thatDoN
/UndoN
are actual functions (and the same signature), requires stack space, slower.
– Acorn
2 days ago
add a comment |
up vote
-2
down vote
up vote
-2
down vote
typedef void(*undoer)();
int undo( undoer*const* list ) {
while(*list) {
(*list)();
++list;
}
}
void undo_push( undoer** list, undoer* undo ) {
if (!undo) return;
// swap
undoer* tmp = *list;
*list = undo;
undo = tmp;
undo_push( list+1, undo );
}
int func() {
undoer undoers[6]={0};
if (Do1()) { printf("Failed 1"); return 1; }
undo_push( undoers, Undo1 );
if (Do2()) { undo(undoers); printf("Failed 2"); return 2; }
undo_push( undoers, Undo2 );
if (Do3()) { undo(undoers); printf("Failed 3"); return 3; }
undo_push( undoers, Undo3 );
if (Do4()) { undo(undoers); printf("Failed 4"); return 4; }
undo_push( undoers, Undo4 );
if (Do5()) { undo(undoers); printf("Failed 5"); return 5; }
return 6;
}
I made undo_push
do the O(n) work. This is less efficient than having undo
do the O(n) work, as we expect more push's than undos. But this version was a touch simpler.
A more industrial strength version would have head and tail pointers and even capacity.
The basic idea is to keep a queue of undo actions in a stack, then execute them if you need to clean up.
Everything is local here, so we don't pollute global state.
struct undoer {
void(*action)(void*);
void(*cleanup)(void*);
void* state;
};
struct undoers {
undoer* top;
undoer buff[5];
};
void undo( undoers u ) {
while (u.top != buff)
{
(u.top->action)(u.top->state);
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pundo(void* pu) {
undo( *(undoers*)pu );
free(pu);
}
void cleanup_undoers(undoers u) {
while (u.top != buff)
{
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pcleanup_undoers(void* pu) {
cleanup_undoers(*(undoers*)pu);
free(pu);
}
void push_undoer( undoers* to_here, undoer u ) {
if (to_here->top != (to_here->buff+5))
{
to_here->top = u;
++(to_here->top)
return;
}
undoers* chain = (undoers*)malloc( sizeof(undoers) );
memcpy(chain, to_here, sizeof(undoers));
memset(to_here, 0, sizeof(undoers));
undoer chainer;
chainer.action = pundo;
chainer.cleanup = pcleanup_undoers;
chainer.data = chain;
push_undoer( to_here, chainer );
push_undoer( to_here, u );
}
void paction( void* p ) {
(void)(*a)() = ((void)(*)());
a();
}
void push_undo( undoers* to_here, void(*action)() ) {
undor u;
u.action = paction;
u.cleanup = 0;
u.data = (void*)action;
push_undoer(to_here, u);
}
then you get:
int func() {
undoers u={0};
if (Do1()) { printf("Failed 1"); return 1; }
push_undo( &u, Undo1 );
if (Do2()) { undo(u); printf("Failed 2"); return 2; }
push_undo( &u, Undo2 );
if (Do3()) { undo(u); printf("Failed 3"); return 3; }
push_undo( &u, Undo3 );
if (Do4()) { undo(u); printf("Failed 4"); return 4; }
push_undo( &u, Undo4 );
if (Do5()) { undo(u); printf("Failed 5"); return 5; }
cleanup_undoers(u);
return 6;
}
but that is getting ridiculous.
typedef void(*undoer)();
int undo( undoer*const* list ) {
while(*list) {
(*list)();
++list;
}
}
void undo_push( undoer** list, undoer* undo ) {
if (!undo) return;
// swap
undoer* tmp = *list;
*list = undo;
undo = tmp;
undo_push( list+1, undo );
}
int func() {
undoer undoers[6]={0};
if (Do1()) { printf("Failed 1"); return 1; }
undo_push( undoers, Undo1 );
if (Do2()) { undo(undoers); printf("Failed 2"); return 2; }
undo_push( undoers, Undo2 );
if (Do3()) { undo(undoers); printf("Failed 3"); return 3; }
undo_push( undoers, Undo3 );
if (Do4()) { undo(undoers); printf("Failed 4"); return 4; }
undo_push( undoers, Undo4 );
if (Do5()) { undo(undoers); printf("Failed 5"); return 5; }
return 6;
}
I made undo_push
do the O(n) work. This is less efficient than having undo
do the O(n) work, as we expect more push's than undos. But this version was a touch simpler.
A more industrial strength version would have head and tail pointers and even capacity.
The basic idea is to keep a queue of undo actions in a stack, then execute them if you need to clean up.
Everything is local here, so we don't pollute global state.
struct undoer {
void(*action)(void*);
void(*cleanup)(void*);
void* state;
};
struct undoers {
undoer* top;
undoer buff[5];
};
void undo( undoers u ) {
while (u.top != buff)
{
(u.top->action)(u.top->state);
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pundo(void* pu) {
undo( *(undoers*)pu );
free(pu);
}
void cleanup_undoers(undoers u) {
while (u.top != buff)
{
if (u.top->cleanup)
(u.top->cleanup)(u.top->state);
--u.top;
}
}
void pcleanup_undoers(void* pu) {
cleanup_undoers(*(undoers*)pu);
free(pu);
}
void push_undoer( undoers* to_here, undoer u ) {
if (to_here->top != (to_here->buff+5))
{
to_here->top = u;
++(to_here->top)
return;
}
undoers* chain = (undoers*)malloc( sizeof(undoers) );
memcpy(chain, to_here, sizeof(undoers));
memset(to_here, 0, sizeof(undoers));
undoer chainer;
chainer.action = pundo;
chainer.cleanup = pcleanup_undoers;
chainer.data = chain;
push_undoer( to_here, chainer );
push_undoer( to_here, u );
}
void paction( void* p ) {
(void)(*a)() = ((void)(*)());
a();
}
void push_undo( undoers* to_here, void(*action)() ) {
undor u;
u.action = paction;
u.cleanup = 0;
u.data = (void*)action;
push_undoer(to_here, u);
}
then you get:
int func() {
undoers u={0};
if (Do1()) { printf("Failed 1"); return 1; }
push_undo( &u, Undo1 );
if (Do2()) { undo(u); printf("Failed 2"); return 2; }
push_undo( &u, Undo2 );
if (Do3()) { undo(u); printf("Failed 3"); return 3; }
push_undo( &u, Undo3 );
if (Do4()) { undo(u); printf("Failed 4"); return 4; }
push_undo( &u, Undo4 );
if (Do5()) { undo(u); printf("Failed 5"); return 5; }
cleanup_undoers(u);
return 6;
}
but that is getting ridiculous.
edited 2 days ago
answered 2 days ago
Yakk - Adam Nevraumont
179k19186365
179k19186365
1
More complex, requires thatDoN
/UndoN
are actual functions (and the same signature), requires stack space, slower.
– Acorn
2 days ago
add a comment |
1
More complex, requires thatDoN
/UndoN
are actual functions (and the same signature), requires stack space, slower.
– Acorn
2 days ago
1
1
More complex, requires that
DoN
/UndoN
are actual functions (and the same signature), requires stack space, slower.– Acorn
2 days ago
More complex, requires that
DoN
/UndoN
are actual functions (and the same signature), requires stack space, slower.– Acorn
2 days ago
add a comment |
up vote
-4
down vote
Let's try for something with zero curly braces:
int result;
result = Do1() ? 1 : 0;
result = result ? result : Do2() ? 2 : 0;
result = result ? result : Do3() ? 3 : 0;
result = result ? result : Do4() ? 4 : 0;
result = result ? result : Do5() ? 5 : 0;
result > 4 ? (Undo5(),0) : 0;
result > 3 ? Undo4() : 0;
result > 2 ? Undo3() : 0;
result > 1 ? Undo2() : 0;
result > 0 ? Undo1() : 0;
result ? printf("Failed %drn", result) : 0;
Yes. 0;
is a valid statement in C (and C++). In the case that some of the functions return something that is incompatible with this syntax (e.g. void perhaps) then the Undo5() style can be used.
This assumes theUndoN
functions return values, when in fact they may be (and most probably are) declaredvoid
(or aren't even functions at all).
– Cássio Renan
2 days ago
msvc is never a particularly standards compliant compiler, but without thinking about it I did actually develop this with void Undo functions. No idea if its actually valid. If it isn't one could just go with: `result > 4 ? (Undo5(), 0) : 0; Doesn't help of course. if 'UndoX' isn't actually a function.
– Chris Becke
2 days ago
yeah, MSVC is a bad, bad boy, for C++ at least. In C, this seems to be valid. My bad.
– Cássio Renan
2 days ago
2
I would argue thatif (result > 4) Undo5();
is easier to understand than a ternary conditional with no false action and a discarded result. (if
statements don't need curly braces)
– pizzapants184
2 days ago
True. I really was avoiding any kind of explicit flow control. ternary conditions are cheating I know.
– Chris Becke
yesterday
add a comment |
up vote
-4
down vote
Let's try for something with zero curly braces:
int result;
result = Do1() ? 1 : 0;
result = result ? result : Do2() ? 2 : 0;
result = result ? result : Do3() ? 3 : 0;
result = result ? result : Do4() ? 4 : 0;
result = result ? result : Do5() ? 5 : 0;
result > 4 ? (Undo5(),0) : 0;
result > 3 ? Undo4() : 0;
result > 2 ? Undo3() : 0;
result > 1 ? Undo2() : 0;
result > 0 ? Undo1() : 0;
result ? printf("Failed %drn", result) : 0;
Yes. 0;
is a valid statement in C (and C++). In the case that some of the functions return something that is incompatible with this syntax (e.g. void perhaps) then the Undo5() style can be used.
This assumes theUndoN
functions return values, when in fact they may be (and most probably are) declaredvoid
(or aren't even functions at all).
– Cássio Renan
2 days ago
msvc is never a particularly standards compliant compiler, but without thinking about it I did actually develop this with void Undo functions. No idea if its actually valid. If it isn't one could just go with: `result > 4 ? (Undo5(), 0) : 0; Doesn't help of course. if 'UndoX' isn't actually a function.
– Chris Becke
2 days ago
yeah, MSVC is a bad, bad boy, for C++ at least. In C, this seems to be valid. My bad.
– Cássio Renan
2 days ago
2
I would argue thatif (result > 4) Undo5();
is easier to understand than a ternary conditional with no false action and a discarded result. (if
statements don't need curly braces)
– pizzapants184
2 days ago
True. I really was avoiding any kind of explicit flow control. ternary conditions are cheating I know.
– Chris Becke
yesterday
add a comment |
up vote
-4
down vote
up vote
-4
down vote
Let's try for something with zero curly braces:
int result;
result = Do1() ? 1 : 0;
result = result ? result : Do2() ? 2 : 0;
result = result ? result : Do3() ? 3 : 0;
result = result ? result : Do4() ? 4 : 0;
result = result ? result : Do5() ? 5 : 0;
result > 4 ? (Undo5(),0) : 0;
result > 3 ? Undo4() : 0;
result > 2 ? Undo3() : 0;
result > 1 ? Undo2() : 0;
result > 0 ? Undo1() : 0;
result ? printf("Failed %drn", result) : 0;
Yes. 0;
is a valid statement in C (and C++). In the case that some of the functions return something that is incompatible with this syntax (e.g. void perhaps) then the Undo5() style can be used.
Let's try for something with zero curly braces:
int result;
result = Do1() ? 1 : 0;
result = result ? result : Do2() ? 2 : 0;
result = result ? result : Do3() ? 3 : 0;
result = result ? result : Do4() ? 4 : 0;
result = result ? result : Do5() ? 5 : 0;
result > 4 ? (Undo5(),0) : 0;
result > 3 ? Undo4() : 0;
result > 2 ? Undo3() : 0;
result > 1 ? Undo2() : 0;
result > 0 ? Undo1() : 0;
result ? printf("Failed %drn", result) : 0;
Yes. 0;
is a valid statement in C (and C++). In the case that some of the functions return something that is incompatible with this syntax (e.g. void perhaps) then the Undo5() style can be used.
edited 2 days ago
Peter Mortensen
13.3k1983111
13.3k1983111
answered 2 days ago
Chris Becke
26.3k656120
26.3k656120
This assumes theUndoN
functions return values, when in fact they may be (and most probably are) declaredvoid
(or aren't even functions at all).
– Cássio Renan
2 days ago
msvc is never a particularly standards compliant compiler, but without thinking about it I did actually develop this with void Undo functions. No idea if its actually valid. If it isn't one could just go with: `result > 4 ? (Undo5(), 0) : 0; Doesn't help of course. if 'UndoX' isn't actually a function.
– Chris Becke
2 days ago
yeah, MSVC is a bad, bad boy, for C++ at least. In C, this seems to be valid. My bad.
– Cássio Renan
2 days ago
2
I would argue thatif (result > 4) Undo5();
is easier to understand than a ternary conditional with no false action and a discarded result. (if
statements don't need curly braces)
– pizzapants184
2 days ago
True. I really was avoiding any kind of explicit flow control. ternary conditions are cheating I know.
– Chris Becke
yesterday
add a comment |
This assumes theUndoN
functions return values, when in fact they may be (and most probably are) declaredvoid
(or aren't even functions at all).
– Cássio Renan
2 days ago
msvc is never a particularly standards compliant compiler, but without thinking about it I did actually develop this with void Undo functions. No idea if its actually valid. If it isn't one could just go with: `result > 4 ? (Undo5(), 0) : 0; Doesn't help of course. if 'UndoX' isn't actually a function.
– Chris Becke
2 days ago
yeah, MSVC is a bad, bad boy, for C++ at least. In C, this seems to be valid. My bad.
– Cássio Renan
2 days ago
2
I would argue thatif (result > 4) Undo5();
is easier to understand than a ternary conditional with no false action and a discarded result. (if
statements don't need curly braces)
– pizzapants184
2 days ago
True. I really was avoiding any kind of explicit flow control. ternary conditions are cheating I know.
– Chris Becke
yesterday
This assumes the
UndoN
functions return values, when in fact they may be (and most probably are) declared void
(or aren't even functions at all).– Cássio Renan
2 days ago
This assumes the
UndoN
functions return values, when in fact they may be (and most probably are) declared void
(or aren't even functions at all).– Cássio Renan
2 days ago
msvc is never a particularly standards compliant compiler, but without thinking about it I did actually develop this with void Undo functions. No idea if its actually valid. If it isn't one could just go with: `result > 4 ? (Undo5(), 0) : 0; Doesn't help of course. if 'UndoX' isn't actually a function.
– Chris Becke
2 days ago
msvc is never a particularly standards compliant compiler, but without thinking about it I did actually develop this with void Undo functions. No idea if its actually valid. If it isn't one could just go with: `result > 4 ? (Undo5(), 0) : 0; Doesn't help of course. if 'UndoX' isn't actually a function.
– Chris Becke
2 days ago
yeah, MSVC is a bad, bad boy, for C++ at least. In C, this seems to be valid. My bad.
– Cássio Renan
2 days ago
yeah, MSVC is a bad, bad boy, for C++ at least. In C, this seems to be valid. My bad.
– Cássio Renan
2 days ago
2
2
I would argue that
if (result > 4) Undo5();
is easier to understand than a ternary conditional with no false action and a discarded result. (if
statements don't need curly braces)– pizzapants184
2 days ago
I would argue that
if (result > 4) Undo5();
is easier to understand than a ternary conditional with no false action and a discarded result. (if
statements don't need curly braces)– pizzapants184
2 days ago
True. I really was avoiding any kind of explicit flow control. ternary conditions are cheating I know.
– Chris Becke
yesterday
True. I really was avoiding any kind of explicit flow control. ternary conditions are cheating I know.
– Chris Becke
yesterday
add a comment |
up vote
-6
down vote
A sane (no gotos, no nested or chained ifs) approach would be
int bar(void)
{
int rc = 0;
do
{
if (do1())
{
rc = 1;
break;
}
if (do2())
{
rc = 2;
break;
}
...
if (do5())
{
rc = 5;
break;
}
} while (0);
if (rc)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
10
If your definition of "sane" is "no goto" then you already failed because sanest way to handle this is indeed to use goto.
– Purple Ice
2 days ago
@PurpleIce: You are probably right for simple cases like the OP's. But the moment we have several such things woven into each other using goto is far to error prone. And yes, this latter case could be considered a design issue.
– alk
2 days ago
3
@alk Thegoto
solution scales linearly to any complexity, as shown in other answers, just like this one, but with less clutter and extraneous loops and branches.
– Acorn
2 days ago
3
Thedo { ... } while (0)
used here is just an obfuscated way of writing agoto
. There’s no advantage at all compared to usinggoto
, and it’s quite a bit harder to read.
– NobodyNada
2 days ago
add a comment |
up vote
-6
down vote
A sane (no gotos, no nested or chained ifs) approach would be
int bar(void)
{
int rc = 0;
do
{
if (do1())
{
rc = 1;
break;
}
if (do2())
{
rc = 2;
break;
}
...
if (do5())
{
rc = 5;
break;
}
} while (0);
if (rc)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
10
If your definition of "sane" is "no goto" then you already failed because sanest way to handle this is indeed to use goto.
– Purple Ice
2 days ago
@PurpleIce: You are probably right for simple cases like the OP's. But the moment we have several such things woven into each other using goto is far to error prone. And yes, this latter case could be considered a design issue.
– alk
2 days ago
3
@alk Thegoto
solution scales linearly to any complexity, as shown in other answers, just like this one, but with less clutter and extraneous loops and branches.
– Acorn
2 days ago
3
Thedo { ... } while (0)
used here is just an obfuscated way of writing agoto
. There’s no advantage at all compared to usinggoto
, and it’s quite a bit harder to read.
– NobodyNada
2 days ago
add a comment |
up vote
-6
down vote
up vote
-6
down vote
A sane (no gotos, no nested or chained ifs) approach would be
int bar(void)
{
int rc = 0;
do
{
if (do1())
{
rc = 1;
break;
}
if (do2())
{
rc = 2;
break;
}
...
if (do5())
{
rc = 5;
break;
}
} while (0);
if (rc)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
A sane (no gotos, no nested or chained ifs) approach would be
int bar(void)
{
int rc = 0;
do
{
if (do1())
{
rc = 1;
break;
}
if (do2())
{
rc = 2;
break;
}
...
if (do5())
{
rc = 5;
break;
}
} while (0);
if (rc)
{
/* More or less stolen from Chris' answer:
https://stackoverflow.com/a/53444967/694576) */
switch(rc - 1)
{
case 5: /* Not needed for this example, but left in in case we'd add do6() ... */
undo5();
case 4:
undo4();
case 3:
undo3();
case 2:
undo2();
case 1:
undo1();
default:
break;
}
}
return rc;
}
answered 2 days ago
alk
57.7k759167
57.7k759167
10
If your definition of "sane" is "no goto" then you already failed because sanest way to handle this is indeed to use goto.
– Purple Ice
2 days ago
@PurpleIce: You are probably right for simple cases like the OP's. But the moment we have several such things woven into each other using goto is far to error prone. And yes, this latter case could be considered a design issue.
– alk
2 days ago
3
@alk Thegoto
solution scales linearly to any complexity, as shown in other answers, just like this one, but with less clutter and extraneous loops and branches.
– Acorn
2 days ago
3
Thedo { ... } while (0)
used here is just an obfuscated way of writing agoto
. There’s no advantage at all compared to usinggoto
, and it’s quite a bit harder to read.
– NobodyNada
2 days ago
add a comment |
10
If your definition of "sane" is "no goto" then you already failed because sanest way to handle this is indeed to use goto.
– Purple Ice
2 days ago
@PurpleIce: You are probably right for simple cases like the OP's. But the moment we have several such things woven into each other using goto is far to error prone. And yes, this latter case could be considered a design issue.
– alk
2 days ago
3
@alk Thegoto
solution scales linearly to any complexity, as shown in other answers, just like this one, but with less clutter and extraneous loops and branches.
– Acorn
2 days ago
3
Thedo { ... } while (0)
used here is just an obfuscated way of writing agoto
. There’s no advantage at all compared to usinggoto
, and it’s quite a bit harder to read.
– NobodyNada
2 days ago
10
10
If your definition of "sane" is "no goto" then you already failed because sanest way to handle this is indeed to use goto.
– Purple Ice
2 days ago
If your definition of "sane" is "no goto" then you already failed because sanest way to handle this is indeed to use goto.
– Purple Ice
2 days ago
@PurpleIce: You are probably right for simple cases like the OP's. But the moment we have several such things woven into each other using goto is far to error prone. And yes, this latter case could be considered a design issue.
– alk
2 days ago
@PurpleIce: You are probably right for simple cases like the OP's. But the moment we have several such things woven into each other using goto is far to error prone. And yes, this latter case could be considered a design issue.
– alk
2 days ago
3
3
@alk The
goto
solution scales linearly to any complexity, as shown in other answers, just like this one, but with less clutter and extraneous loops and branches.– Acorn
2 days ago
@alk The
goto
solution scales linearly to any complexity, as shown in other answers, just like this one, but with less clutter and extraneous loops and branches.– Acorn
2 days ago
3
3
The
do { ... } while (0)
used here is just an obfuscated way of writing a goto
. There’s no advantage at all compared to using goto
, and it’s quite a bit harder to read.– NobodyNada
2 days ago
The
do { ... } while (0)
used here is just an obfuscated way of writing a goto
. There’s no advantage at all compared to using goto
, and it’s quite a bit harder to read.– NobodyNada
2 days ago
add a comment |
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f53444743%2fclean-ways-to-do-multiple-undos-in-c%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
1
Just to point out with regard to my previous comment: in these cases, it is generally the case that these cleanup actions need to be also performed when there are no early abortions (hence the mention of free/fclose); that makes the structure with goto & labels fairly straightforward and easy to read. This may not be the case that you are thinking of.
– 9769953
2 days ago
2
I believe you need to clarify the types of the functions, as we are getting all kinds of mixed answers. Are they the same type, or are all functions of different types?
– Lundin
2 days ago
1
@Lundin It is clear the code is an example of the code patterns that appear in many C functions, typically those that allocate resources. Precisely because the types do not matter here, they are not specified. It is pseudocode, if you want.
– Acorn
2 days ago
5
@Lundin No, I haven't said anything like it. And no, pseudo-code can be perfectly on topic.
– Acorn
2 days ago
3
@9769953 - I'd say the problem wasn't
goto fail;
so much as avoiding curly braces. And it's not like it's a new sort of bug. The offending line didn't have to be agoto
, but could just as easily have been aexit(EXIT_FAILURE)
. Still the same bug, despite the different manifestation.– StoryTeller
2 days ago