Comment by unwind | Hacker News Reader

Comment by unwind | Hacker News Reader

Parent Full thread

This is very well written, and it seems the author has already used the great code reviewing powers of the Internet (there are references to improvements from Reddit comments).

I suffer from compulsory C review disorder, so just a few quick points:

- The pattern "x = realloc(x, ...)" is a bad idea, it leaks the original memory on failure. Since the code seems to car about its memory management (and since it's a tutorial), I think this is worth mentioning (and fixing).

- I generally recommend against scaling string allocations by sizeof (char); that's always 1 so it doesn't do anything. Some people seem to feel that it's part of a general usage pattern that makes the intent more clear, though.

- I would recommend more "const" and "static", but some people seem to not care.

- I would use more size_t rather than int for things that are sizes. This is more clear, and also safer since it's unsigned.

- Using strtok() is not always the best choice, it has issues, but here it's basically doing what it was designed to do I guess.

- The pattern "x = realloc(x, ...)" is a bad idea, it leaks the original memory on failure. Since the code seems to car about its memory management (and since it's a tutorial), I think this is worth mentioning (and fixing).

If you look at what the code does after that...

      if (!buffer) {
        fprintf(stderr, "lsh: allocation error\n");
        exit(EXIT_FAILURE);
      }

...there is nothing to "fix" because there is nothing more the code can do --- the whole process is gone after that; unless you imply getting into the intricacies of trying to recover from out-of-memory situations, which quickly opens its own huge can of worms (does fprintf() itself do an allocation? Could that allocation fail? What to do if printing the error message fails? Etc. etc.)

In general, I think the "this pattern is bad" sort of "thinking" is itself a bad idea; when writing and examining code, you should be looking at each situation itself and thinking to evaluate it, not just trying to match everything into one "pattern" or another. Does "x = realloc(x, ...)" make sense in all contexts? No. Does it make sense here? Yes. Otherwise it is a path towards mindless cargo-cultism, something which beginners should certainly be discouraged from doing.

Yeah, good point about the exit(), I realized that and mentioned it in a later comment (below).

I do really try to look at the situation, but that doesn't change the fact (at least in my experience) that there are patterns of "bad code" that seem to have a life of their own.

Probably caused by things being unintuitive, and/or more complex than people first realize, or something.

I'm really opposed to cargo cult programming, so no argument there. :)

> The pattern "x = realloc(x, ...)" is a bad idea, it leaks the original memory on failure. Since the code seems to car about its memory management (and since it's a tutorial), I think this is worth mentioning (and fixing).

What would the alternative be here? Allocate a new region, copy everything over and free the other region? Or is there something better?

> I generally recommend against scaling string allocations by sizeof (char); that's always 1 so it doesn't do anything. Some people seem to feel that it's part of a general usage pattern that makes the intent more clear, though.

Isn't this just a habit born out of portability concerns?

The alternative is to use a separate variable:

    y = realloc(x, ...);

Then if it fails, you can still free(x) before continuing. Of course, if you're going to exit() due to the failure it doesn't matter and perhaps that's why the original code looks that way. I still think it's worth pointing out, though.

About sizeof (char), there can be no "portability concerns", and that very thing is what I think gives rise to a lot of (in my opinion, bad) cargo cult programming. The value of "sizeof (char)" in C is 1. Note that I didn't say "using GCC x.y.z on x86". It's true for all conforming compilers, on all platforms, and on all architectures. It's defined by the language.

  > The value of "sizeof (char)" in C is 1.

For those who might not be aware, this does not mean that a char is always 8 bits.

According to C99 spec, the maximum number of bits for the smallest object that is not a bit-field is 8 bits. See "CHAR_BIT" in the C99 spec.

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Smallest. It might be larger (and infact it is in a lot of DSP toolchains)

Regarding realloc:

    y = realloc(x, ...)
    if (y == null) {
       free(x);
       // Handle OOM situation
    }
    x = y;

> What would the alternative be here? Allocate a new region, copy everything over and free the other region? Or is there something better?

No.

If realloc fails it returns NULL. In which case the original `x` is still valid. ==> However with `x = realloc(x,...)` you don't don't have a reference to `x` any more!

Reply on news.ycombinator.com