Skip to main content
copy-edited
Source Link

After some reflection on user Kevin's post hereKevin's post here, I thought it might be useful to capture some of the comment discussion permanently and also provide to users who don't use C very much (I don't myself lately), a deeper explanation of what is going on.

int read_some_type(char *filename, struct some_type *result){
    int rc = 0;
    { //<1> "Code is broken on entry" shell
        if(!result){
            rc = -1;
            goto out;
        }
        { //<2> "Try to open a file" shell
            int fd = open(filename, O_RDONLY);
            if(fd == -1){
                rc = -1;
                goto out;
            }
            { //<3> "Try to allocate" shell
                // Normally we'd allocate this on the stack; assume struct some_type is huge.
                char *str = malloc(sizeof(struct some_type));
                if(!str){
                    rc = -1;
                    goto out_open;
                }
                { //<4> "Evaluate and act" shell
                    ssize_t size;
                    size_t total = 0;
                    while(total != sizeof(struct some_type)){
                        size = read(fd, &str[total], total - sizeof(struct some_type));
                        if(size <= 0){
                            rc = -1;
                            goto out_malloc;
                        }
                        total += size;
                    }
                    memcpy(result, str, sizeof(struct some_type));
                    out_malloc:
                } //</4>
                free(str);
                out_open:
            } //</3> 
            close(fd);
        out:
        } //</2>
    } //</1>
    // This is strictly unnecessary, but we could add code here in the future.
    // For example, we might check errno and call perror(3) or similar.
    return rc;
}
int read_some_type(char *filename, struct some_type *result){
    int rc = 0;
    { //<1> "Code is broken on entry" shell
        if(!result){
            rc = -1;
            goto out;
        }
        { //<2> "Try to open a file" shell
            int fd = open(filename, O_RDONLY);
            if(fd == -1){
                rc = -1;
                goto out;
            }
            { //<3> "Try to allocate" shell
                // Normally we'd allocate this on the stack; assume struct some_type is huge.
                char *str = malloc(sizeof(struct some_type));
                if(!str){
                    rc = -1;
                    goto out_open;
                }
                { //<4> "Evaluate and act" shell
                    ssize_t size;
                    size_t total = 0;
                    while(total != sizeof(struct some_type)){
                        size = read(fd, &str[total], total - sizeof(struct some_type));
                        if(size <= 0){
                            rc = -1;
                            goto out_malloc;
                        }
                        total += size;
                    }
                    memcpy(result, str, sizeof(struct some_type));
                    out_malloc:
                } //</4>
                free(str);
                out_open:
            } //</3> 
            close(fd);
        out:
        } //</2>
    } //</1>
    // This is strictly unnecessary, but we could add code here in the future.
    // For example, we might check errno and call perror(3) or similar.
    return rc;
}

After some reflection on user Kevin's post here, I thought it might be useful to capture some of the comment discussion permanently and also provide to users who don't use C very much (I don't myself lately), a deeper explanation of what is going on.

int read_some_type(char *filename, struct some_type *result){
    int rc = 0;
    { //<1> "Code is broken on entry" shell
        if(!result){
            rc = -1;
            goto out;
        }
        { //<2> "Try to open a file" shell
            int fd = open(filename, O_RDONLY);
            if(fd == -1){
                rc = -1;
                goto out;
            }
            { //<3> "Try to allocate" shell
                // Normally we'd allocate this on the stack; assume struct some_type is huge.
                char *str = malloc(sizeof(struct some_type));
                if(!str){
                    rc = -1;
                    goto out_open;
                }
                { //<4> "Evaluate and act" shell
                    ssize_t size;
                    size_t total = 0;
                    while(total != sizeof(struct some_type)){
                        size = read(fd, &str[total], total - sizeof(struct some_type));
                        if(size <= 0){
                            rc = -1;
                            goto out_malloc;
                        }
                        total += size;
                    }
                    memcpy(result, str, sizeof(struct some_type));
                    out_malloc:
                } //</4>
                free(str);
                out_open:
            } //</3> 
            close(fd);
        out:
        } //</2>
    } //</1>
    // This is strictly unnecessary, but we could add code here in the future.
    // For example, we might check errno and call perror(3) or similar.
    return rc;
}

After some reflection on user Kevin's post here, I thought it might be useful to capture some of the comment discussion permanently and also provide to users who don't use C very much (I don't myself lately), a deeper explanation of what is going on.

int read_some_type(char *filename, struct some_type *result){
    int rc = 0;
    { //<1> "Code is broken on entry" shell
        if(!result){
            rc = -1;
            goto out;
        }
        { //<2> "Try to open a file" shell
            int fd = open(filename, O_RDONLY);
            if(fd == -1){
                rc = -1;
                goto out;
            }
            { //<3> "Try to allocate" shell
                // Normally we'd allocate this on the stack; assume struct some_type is huge.
                char *str = malloc(sizeof(struct some_type));
                if(!str){
                    rc = -1;
                    goto out_open;
                }
                { //<4> "Evaluate and act" shell
                    ssize_t size;
                    size_t total = 0;
                    while(total != sizeof(struct some_type)){
                        size = read(fd, &str[total], total - sizeof(struct some_type));
                        if(size <= 0){
                            rc = -1;
                            goto out_malloc;
                        }
                        total += size;
                    }
                    memcpy(result, str, sizeof(struct some_type));
                    out_malloc:
                } //</4>
                free(str);
                out_open:
            } //</3> 
            close(fd);
        out:
        } //</2>
    } //</1>
    // This is strictly unnecessary, but we could add code here in the future.
    // For example, we might check errno and call perror(3) or similar.
    return rc;
}
added 187 characters in body
Source Link
Buffy
  • 36.8k
  • 10
  • 66
  • 117

It is also important to note that each shell cleans up after itself. So both allocation and the needed deallocation take place within the same shell. Similarly for the file handling.

Moreover, if you are going to code this way you need to be absolutely pure in your implementation of it. It is fine to add a new shell, inner to another as long as you keep the same discipline. You don't need to create the extra blocks as I've done here, but you do need to write as if they exist. If you use additional goto statements to jump between the shells/blocks you will have a mess.

Moreover, if you are going to code this way you need to be absolutely pure in your implementation of it. It is fine to add a new shell, inner to another as long as you keep the same discipline. You don't need to create the extra blocks as I've done here, but you do need to write as if they exist. If you use additional goto statements to jump between the shells/blocks you will have a mess.

It is also important to note that each shell cleans up after itself. So both allocation and the needed deallocation take place within the same shell. Similarly for the file handling.

Moreover, if you are going to code this way you need to be absolutely pure in your implementation of it. It is fine to add a new shell, inner to another as long as you keep the same discipline. You don't need to create the extra blocks as I've done here, but you do need to write as if they exist. If you use additional goto statements to jump between the shells/blocks you will have a mess.

Source Link
Buffy
  • 36.8k
  • 10
  • 66
  • 117

After some reflection on user Kevin's post here, I thought it might be useful to capture some of the comment discussion permanently and also provide to users who don't use C very much (I don't myself lately), a deeper explanation of what is going on.

I agree with Kevin (hereafter the OP = original poster) that this sort of coding isn't what you teach novices first, and is limited, perhaps, to C-like (even assembly-like) languages. Below, I've modified his example just a bit by trying to explicate the various shells that I discussed in a comment. In particular, the only change has been to add a block structure to the code using braces (which can be opened anywhere and create a new lexical level, though I don't need to use if for renaming variables here). It is just to make the structure (I hope) clearer to the reader and the code archaeologist. I've also given "names" to the blocks/levels/shells, though just as comments.

int read_some_type(char *filename, struct some_type *result){
    int rc = 0;
    { //<1> "Code is broken on entry" shell
        if(!result){
            rc = -1;
            goto out;
        }
        { //<2> "Try to open a file" shell
            int fd = open(filename, O_RDONLY);
            if(fd == -1){
                rc = -1;
                goto out;
            }
            { //<3> "Try to allocate" shell
                // Normally we'd allocate this on the stack; assume struct some_type is huge.
                char *str = malloc(sizeof(struct some_type));
                if(!str){
                    rc = -1;
                    goto out_open;
                }
                { //<4> "Evaluate and act" shell
                    ssize_t size;
                    size_t total = 0;
                    while(total != sizeof(struct some_type)){
                        size = read(fd, &str[total], total - sizeof(struct some_type));
                        if(size <= 0){
                            rc = -1;
                            goto out_malloc;
                        }
                        total += size;
                    }
                    memcpy(result, str, sizeof(struct some_type));
                    out_malloc:
                } //</4>
                free(str);
                out_open:
            } //</3> 
            close(fd);
        out:
        } //</2>
    } //</1>
    // This is strictly unnecessary, but we could add code here in the future.
    // For example, we might check errno and call perror(3) or similar.
    return rc;
}

Each "shell" is now a block. Each shell tries to do something that might fail. Each shell ends with a label. Each "goto" is essentially a return from the shell, as was noted by the OP and I also noticed. Each shell is immediately followed by cleanup from that shell. This is a special case, actually, since here the cleanup needs to be done both on success and failure of the overall code. If the cleanup were only needed on failure, the innermost shell could do an immediate return after success is achieved, though goto out here is equivalent as there is no code following that label.

If you think about the code, while it doesn't use "helper" functions, in effect what it is is a bunch of helper functions expanded in place (inlined) rather than called explicitly. The lexical structure replaces arguments. This sort of thing (inlining) is often used for efficiency (and making Torvalds happy, I guess). But in most coding efficiency is the last thing you need to consider (after writing clearer code and then running a profiler to see where efficiency suffers). Kernel code in an OS is an obvious exception, of course.

Moreover, if you are going to code this way you need to be absolutely pure in your implementation of it. It is fine to add a new shell, inner to another as long as you keep the same discipline. You don't need to create the extra blocks as I've done here, but you do need to write as if they exist. If you use additional goto statements to jump between the shells/blocks you will have a mess.

Also, if you treat the goto statements as "returns" from the shell, then the idea of "returning as soon as you can" is still in place. Here, you "return" when you notice a failure (say of malloc). But you also "return" at the end of the shell by falling through to the outer shell.


The code is also a bit special since two of the shells (#1 and #2) end at the same place, though that isn't the general situation. In general there might be two different targets for what is, here, the out label. That label would be after the end of the #2 shell at the end of #1


Note this post is "owned" by the community so you are free to improve it as well as comment on it. In particular, I'd be interested in hearing from the OP whether making these shells/levels explicit would be considered to be a good or bad thing in his environment.

Post Made Community Wiki by Buffy