Comment by unwind | Hacker News Reader

Comment by unwind | Hacker News Reader

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)... 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.

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; }

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.

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.

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.

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".)

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.

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.

I would say ONE_TB is an improvement over 1099511627776 when it comes to "natural to reason about" numbers, but yes, defining ZERO and ONE is moronic.

A lot of times TiB is spelled 1024 * 1024 * 1024 * 1024 or 1 << 40, relying on compile-time constant folding to compute the final value. I usually find that a lot more readable than remembering the actual number.

#define M_PI 3.14159265358979323846 is usually nicer than seeing (and possibly mistyping) those digits of pi each time.

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.

Tell me what the number represents rather than what the value of the number is. How about this?
#define BUFFER_LENGTH 2048 new int[BUFFER_LENGTH];

And Standard C already defines a constant you can use: BUFSIZ.

I haven't read through all of the author's content yet, but there is a certain benefit to using magic numbers in introductory material. Numbers impose very little conceptual overhead on people, especially "familiar" numbers like 2048 or other common buffer sizes. This allows the reader to save their precious meatspace memory for storing the novel concepts the author introduces.
But it depends how the code is presented. "Here's an example of..." is different than "Here's how you should do..."

Also lots of people still use #define for magic numbers; I learned C in the late 80s with a book by Thomas Plum, and it recommended using enum whenever possible in preference of #define.
The reason given was superior debug support. Debuggers have gotten better at handling #defines, but #defines are still second class citizens. I've personally worked with a debugger that could precisely find all references of an enum, but only grep for a #define. In code with lots of conditional compilation, that makes a huge difference.

Reply on news.ycombinator.com