4 ms·

I always find it so hard to help out proof-reading code for causes like this. Often, the code is more complicated than I find reasonable, while omitting things

by unwind 13y ago

I always find it so hard to help out proof-reading code for causes like this. Often, the code is more complicated than I find reasonable, while omitting things that make a lot of sense in "real" code, and it's very hard to know as an outside reader what the exact motivation for each decision was, by the author. A few such things that caused me to WTF: The initial few examples use a pointlessly static and global line buffer, instead of declaring the line buffer where it's being used. There is hardly any const in the code, even for cases where it obviously should (to me) be used, i.e. for variables that are never written once given their initial value. A magic number (the buffer size 2048) is repeated in the code, and even encoded into a comment, instead of just using "sizeof buffer". I do think I found an actual bug (on http://www.buildyourownlisp.com/chapter4_interactive_prompt); http://www.buildyourownlisp.com/chapter4_interactive_prompt)... the readline() implementation ignores its prompt argument and uses a hardcoded string, instead. The same function also does strcpy() followed by using strlen() to truncate the string it just copied; that really doesn't sit well with me.

DSMan195276 13y ago
I would agree, like all code this author definitely has a few quirks about how they write their C code, and I think all of your criticisms are valid. Their readline implementation is definitely sloppy, but they also just throw it in there without really talking about it. Adding to the readline issue, they definitely should have const'd the strings for readline and add_history. It's not just good practice, in this case it'd be required to conform to how readline's declaration is. I'm surprised they don't get warnings about passing a const char * into a char . Also, talking about the strcpy() and strlen(), the catch there is that it's actually removing the '\n' from fgets, it's not actually trying to terminate the string. That said, they should have just saved the strlen(buffer) result in a size_t and then used it later. The real function should look something like this: /* Fake readline function */ char* readline(const char* prompt) { fputs(prompt, stdout); fgets(buffer, sizeof buffer, stdin); size_t bufsiz = strlen(buffer); char* cpy = malloc(bufsiz + 1); strcpy(cpy, buffer); cpy[bufsiz - 1] = '\0'; return cpy; }
- unwind 13y ago
  Well, yes, I know what the strlen() is for, that's why I wrote truncate (not terminate); it is shortening the string by 1 character. I see two problems with your code: it's over-allocating, since it's going to do the truncation there's no need to add 1 for the termination, it balances out with the newline; and there's absolutely no point in using strcpy() when you know the length.
  - DSMan195276 13y ago
    Fair enough. It wasn't obvious to me what the strlen was doing at first glance, and I doubt it's obvious to any 'beginners' reading the book so it's worth noting either way. It's true, you're right, I was keeping it a bit in-line with the book, if you change the malloc to be one less, then you can't use strcpy since it'll write the null term to memory you don't have (It might actually work on some platforms since it's just one byte but it's undefined behavior). If you just memcpy the string with bufsiz then there's no issue though.
cliveowen 13y ago
If there's one thing that I really can't stand are magic numbers. The K&R says to always use constants, which means this has been a good practice for more than 40 years and I really can't grasp how someone can hope to teach something when his own knowledge lacks the basics.
- michaelochurch 13y ago
  In examples, magic numbers are OK. It's in large production systems (that will require tuning, which becomes impossible with "magic numbers") that they're a code smell. I'd much rather, in a small example, see: new int[2048]; than #define TWENTYFORTYEIGHT 2048 new int[TWENTYFORTYEIGHT]; Also, it's generally OK to use a "magic number" if you know that the constant will only be used in that one place, or that there is no way it will ever change. Where they're bad is when they represent undocumented constraints across a program (i.e. "this 64 and that 64 are the same and can't be changed independently".)
  - john_b 13y ago
    In my experience, aliasing numbers to words has never improved any code in any way whatsoever, and sometimes causes more headaches since numbers are easy and natural to reason about numerically (big surprise...), while values such as TWENTYFORTYEIGHT impose more mental overhead and open you up to typos. It strikes me as a symptom of the "magic numbers are evil witchcraft" religion gone to the extreme. The whole point of banishing magic numbers is to improve code's clarity and robustness to change. I've seen #define ZERO 0 and #define ONE 1 more times than I care to admit. I have yet to see the value of either change.
    - zenlikethat 13y ago
      Yeah, we were always taught that zero, one, empty string and other "edge case" values were O.K. to have in code directly. I'm inclined to agree.
    - theseoafs 13y ago
      Programmers that replace 0 with ZERO, 1 with ONE, and 2048 with TWENTYFORTYEIGHT are severely misunderstanding why magic numbers are a bad thing in the first place. TWENTYFORTYEIGHT means exactly the same thing as 2048 (that is to say, TWENTYFORTYEIGHT means roughly nothing), but replacing 2048 with a symbolic constant that actually means something (say, BUFFER_SIZE) is a good idea.

by unwind 13y ago

DSMan195276 13y ago

I would agree, like all code this author definitely has a few quirks about how they write their C code, and I think all of your criticisms are valid. Their readline implementation is definitely sloppy, but they also just throw it in there without really talking about it. Adding to the readline issue, they definitely should have const'd the strings for readline and add_history. It's not just good practice, in this case it'd be required to conform to how readline's declaration is. I'm surprised they don't get warnings about passing a const char * into a char . Also, talking about the strcpy() and strlen(), the catch there is that it's actually removing the '\n' from fgets, it's not actually trying to terminate the string. That said, they should have just saved the strlen(buffer) result in a size_t and then used it later. The real function should look something like this: /* Fake readline function */ char* readline(const char* prompt) { fputs(prompt, stdout); fgets(buffer, sizeof buffer, stdin); size_t bufsiz = strlen(buffer); char* cpy = malloc(bufsiz + 1); strcpy(cpy, buffer); cpy[bufsiz - 1] = '\0'; return cpy; }

unwind 13y ago
Well, yes, I know what the strlen() is for, that's why I wrote truncate (not terminate); it is shortening the string by 1 character. I see two problems with your code: it's over-allocating, since it's going to do the truncation there's no need to add 1 for the termination, it balances out with the newline; and there's absolutely no point in using strcpy() when you know the length.
- DSMan195276 13y ago
  Fair enough. It wasn't obvious to me what the strlen was doing at first glance, and I doubt it's obvious to any 'beginners' reading the book so it's worth noting either way. It's true, you're right, I was keeping it a bit in-line with the book, if you change the malloc to be one less, then you can't use strcpy since it'll write the null term to memory you don't have (It might actually work on some platforms since it's just one byte but it's undefined behavior). If you just memcpy the string with bufsiz then there's no issue though.

cliveowen 13y ago

If there's one thing that I really can't stand are magic numbers. The K&R says to always use constants, which means this has been a good practice for more than 40 years and I really can't grasp how someone can hope to teach something when his own knowledge lacks the basics.

michaelochurch 13y ago
In examples, magic numbers are OK. It's in large production systems (that will require tuning, which becomes impossible with "magic numbers") that they're a code smell. I'd much rather, in a small example, see: new int[2048]; than #define TWENTYFORTYEIGHT 2048 new int[TWENTYFORTYEIGHT]; Also, it's generally OK to use a "magic number" if you know that the constant will only be used in that one place, or that there is no way it will ever change. Where they're bad is when they represent undocumented constraints across a program (i.e. "this 64 and that 64 are the same and can't be changed independently".)
- john_b 13y ago
  In my experience, aliasing numbers to words has never improved any code in any way whatsoever, and sometimes causes more headaches since numbers are easy and natural to reason about numerically (big surprise...), while values such as TWENTYFORTYEIGHT impose more mental overhead and open you up to typos. It strikes me as a symptom of the "magic numbers are evil witchcraft" religion gone to the extreme. The whole point of banishing magic numbers is to improve code's clarity and robustness to change. I've seen #define ZERO 0 and #define ONE 1 more times than I care to admit. I have yet to see the value of either change.
  - zenlikethat 13y ago
    Yeah, we were always taught that zero, one, empty string and other "edge case" values were O.K. to have in code directly. I'm inclined to agree.
  - theseoafs 13y ago
    Programmers that replace 0 with ZERO, 1 with ONE, and 2048 with TWENTYFORTYEIGHT are severely misunderstanding why magic numbers are a bad thing in the first place. TWENTYFORTYEIGHT means exactly the same thing as 2048 (that is to say, TWENTYFORTYEIGHT means roughly nothing), but replacing 2048 with a symbolic constant that actually means something (say, BUFFER_SIZE) is a good idea.