14 ms·
Very cool, congratulations! I took a 2-second peek at the code, and just wanted to offer a small piece of advice that I think makes it better. Or two, really.
by unwind 2y ago
Very cool, congratulations!
I took a 2-second peek at the code, and just wanted to offer a small piece of advice that I think makes it better. Or two, really. Counting is hard.
Instead of (this is from parser.c):
apply_result *ret = (apply_result*)malloc(sizeof(apply_result));
apply the two common principles of DRY [1] and "don't cast the return value of malloc()" [2] and you get:
apply_result *ret = malloc(sizeof *ret);
I think the latter is way better.
[1]: https://en.wikipedia.org/wiki/Don%27t_repeat_yourself https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
[2]: https://stackoverflow.com/a/605858 https://stackoverflow.com/a/605858
- keyvank 2y agothanks man! yes that makes sense. unfortunately 30cc is not able to compile that syntax, and will probably give type-checker errors when passing a void* to pointer of another type. but will implement it sometime soon!
- unwind 2y agoAha, so it doesn't know about void* being compatible with all (object) pointers, but so far just requires the pointers to match exactly? I see. Not sure why I was down-voted, I think that is ... not nice. :(
- nar001 2y agoI don't think you can be downvoted, there's no downvote button
- hollerith 2y agoUsers with enough karma can downvote comments.
- quuxplusone 2y agoA beautiful example of Cunningham's Law! :)
- dataflow 2y agoI downvoted just because I thought it wasn't great advice. It makes the code incompatible with C++ compilers, and being able to compile C with C++ compiler is often useful. Also... it felt kind of... unnecessary? to tell someone who is making a compiler how to write basic code in the language they're writing a compiler for. It almost felt like telling an overweight medical student/doctor that he should avoid eating fatty foods.
- Gibbon1 2y agoWriting C code so won't compile with a C++ compiler is gods work. Because C++ is why C can't have nice things.
- KerrAvon 2y agoI disagree with that advice, FWIW -- (void *) is the cause of a lot of bugs in C programs, and much stricter type checking was often the norm for compilers like CodeWarrior on platforms like classic Mac OS, where (a) if you got something wrong you'd corrupt app memory at best and the filesystem at worst and (b) many developers were used to Pascal, which was stricter. (Moving to GCC on Mac OS X and unexpectedly getting much more lenient type checking was a big surprise.)
- munch117 2y agoBut adding the cast doesn't make type checking stricter, quite the opposite - it destroys any hope of the compiler checking the assignment. C++ shot itself in the foot by removing automatic conversion from void* to T*, because it forces programmers to add casts that weaken type safety.
- cataphract 2y agoIt doesn't make it any safer because the cast is unchecked. It could be argued it's discouraged[1] for a reason that doesn't really apply to modern C versions where implicit-int isn't allowed anymore. But while the cast is a matter of style (or C++ compatibility), sizeof(*ret) is definitely superior to sizeof(thing_t). The reason is that people sometimes change the type of ret but forget the change the size of the allocation, especially if ret is assigned to on a different line it's declared. [1]: https://c-faq.com/malloc/mallocnocast.html https://c-faq.com/malloc/mallocnocast.html
- KerrAvon 2y agoIf you're implementing C23 compatibility, you could do auto p = (thing_t *)malloc(sizeof(thing_t)); and you get both DRY and avoid (void *) casting.
- dcuthbertson 2y ago> and you get both DRY and avoid (void *) casting. While that line of code is DRY, it doesn't avoid casting "void *" to "thing_t *".
- gizmo 2y agoThis kind of condescending advice doesn't help anybody. Redundant casts, like redundant braces, white space, and a plethora of similar choices are stylistic more than anything else. If you really feel the need to give unsolicited coding style feedback maybe at least spend more than a few seconds looking at the code before you do so. Otherwise it's just rude.
- unwind 2y agoReally? Wow. I had no idea, and I really tried to sound non-condescending. I simply don't agree that redundant casts are something to be ignored, since they can be harmful and hide actual errors (the cast is a bit like `sudo make me a sandwich`, it makes the compiler do what you say which is not always a good idea). I guess I believe that publicly posting code, and even writing a compiler for the language in question, kind of automatically marks you as interested in the language and open for ideas of usage and so on. My apologies to the OP if you were offended. Thanks for the feedback, @gizmo.
- solarengineer 2y agoFWIW, I liked your suggestion, the author’s explanation, And your follow on written understanding. If someone’s technical article is submitted and they are themselves participating in a discussion out feeling insulted, then accusations of unsolicited advice are unfounded.
- williamdclt 2y agoI didn't find your tone condescending at all, it was a perfectly fine comment (esp with sources)
- cschep 2y agoI enjoyed reading the feedback, and their response, and felt no negativity whatsoever. Keep honestly interacting it's good for everyone!
- dominicrose 2y agoGiving feedback - solicited or not - is part of an engineer's job. Wether it's cosmetic feedback or not doesn't matter as cosmetics are also part of the job, although not everybody thinks so.