Jump to content
  • Advertisement
Sign in to follow this  
Gage64

The use of goto in C (not C++)

This topic is 3759 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Say I have this function:
void func() {
    int *arr = (int*)malloc(sizeof(int) * 50);
    if (!arr) {
        printf("Error allocating memory");
freeMem:
        free(arr);
        return;
    }
    
    FILE *file = fopen("myfile.txt", "t");
    if (!file) {
        printf("Failed to open file");
        goto freeMem;
    }
    
    if (some error) {
closeFile:
        fclose(file);
        goto freeMem;
    }
    
    if (some error)
        goto closeFile;
    
    if (some error)
        goto closeFile;
    
    /* ... */
}


I'm not used to programming in straight C, so the code may contain some errors. Do you think the use of goto is justified here? If not, how would you rewrite the code to avoid it? Although it's already stated in the title, I'll say it again - assume that you have to write the function is straight C. You can't use any C++ features. [Edited by - Gage64 on July 3, 2008 11:07:42 AM]

Share this post


Link to post
Share on other sites
Advertisement
I think goto is OK here, but I think you can simplify the logic just a little bit. Something like this would be better:

void func( )
{
int *arr = (int*)malloc(sizeof(int) * 50);
FILE *file = fopen("myfile.txt", "t");

if(!arr || !file)
goto end;

if (some error)
goto end;

if (some error)
goto end;

if (some error)
goto end;

/* ... */

end:
if(arr) free(arr);
if(file) close(file);
}


That is the cleanest thing I can think of without seeing more code. It looks like this function is trying to do too much though. Can you split this up at all?

Share this post


Link to post
Share on other sites
Quote:
Original post by Simian Man
That is the cleanest thing I can think of without seeing more code. It looks like this function is trying to do too much though. Can you split this up at all?


It was just a theoretical question, I don't actually have to write something like this (thankfully [smile]).

And yes, your solution is much nicer, although I think the first if statement is not needed Never mind, I'm an idiot [smile].

Share this post


Link to post
Share on other sites
I do consider this an idiom, so I wouldn't worry about it too much if I encountered it in code. However, I would probably write things as such myself, to further encapsulate the various allocations:


RESULT func()
{
int *buf = malloc(sizeof(int) * 50);
RESULT r;

if (!buf) return ERROR_ALLOC;

r = read_file(buf);

free (arr);
return r;
}

RESULT read_file(int *target)
{
FILE *file = fopen("myfile.txt", "r");
RESULT r;

if (!file) return ERROR_FILE_OPEN;

r = do_funny_things(target, file);

fclose(file);
return r;
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Gage64
And yes, your solution is much nicer, although I think the first if statement is not needed.


Yeah you're right. I knew delete was safe on NULL pointers, but I couldn't remember about free (it is safe).

Share this post


Link to post
Share on other sites
I realize it's not a real world example, but I would assume we only need to allocate memory if we are actually able to open the file.

You could choose to check the pointers at the end of the function:

FILE *file = 0;
int *arr = 0;

if (some error)
/* stuff to handle this specific error */
else if (some error)
/* stuff to handle this specific error */
else if (some error)
/* stuff to handle this specific error */

/* more code.. */

if (file)
fclose(file)

Share this post


Link to post
Share on other sites
Quote:
Original post by Simian Man
Quote:
Original post by Gage64
And yes, your solution is much nicer, although I think the first if statement is not needed.


Yeah you're right. I knew delete was safe on NULL pointers, but I couldn't remember about free (it is safe).


I corrected my post. It is definitely needed because if either one is NULL, the function should obviously not continue executing. BTW, I meant the if statement at the top of the function.

Share this post


Link to post
Share on other sites
Quote:
Original post by WanMaster
You could choose to check the pointers at the end of the function:


I'm not sure what you mean. If you failed to open the file, you have to know about it right away, so how can you check for this only at the end of the function?

Share this post


Link to post
Share on other sites
Quote:
Original post by Gage64
Quote:
Original post by WanMaster
You could choose to check the pointers at the end of the function:


I'm not sure what you mean. If you failed to open the file, you have to know about it right away, so how can you check for this only at the end of the function?

FILE *file = 0;
int *arr = 0;

if (some error) {
display_error("file not found"); }
else if (some error) {
display_error("file in use"); }
else if (some error) {
display_error("permission denied"); }
else {
/* no error, do stuff with file */
arr = (int*)malloc(sizeof(int) * 50);
/* etc. */
}

if (file)
fclose(file) /* of course in this simple case this could be done within the else-scope */




Well, what I meant to demonstrate is having a single point in code where the file is closed (or memory is released) without the need of a label and its jump. Admittedly, as the function body grows in complexity this will become very tricky, but in that case it should have be split up in to multiple procedures anyway.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!