rui314 / chibicc

A small C compiler
MIT License
9.74k stars 892 forks source link

Memory leak #22

Open robinmoussu opened 4 years ago

robinmoussu commented 4 years ago

In a1ab0ff26f23c82f15180051204eeb6279747c9a, a memory leak is introduced. I added one possible way to fix it in 427fb998f44913460124f470d34edb2014108014. I just don't know how to create a PR targeting a given commit instead of a branch.

I am currently trying to re-write the whole compiler in Rust, one commit at a time (I will see if it was wise later!). I notice that (at least in the first commits), you C code isn't really robust against bad input, and would expose undefined behavior. Do you want me to create issues/PR against those?

gggin commented 4 years ago

This project is just let memory leak. See Readme carefully.

robinmoussu commented 4 years ago

I was reading the commits one by one. The Readme doesn't exists yet!

But given that you plan to eventually publish a book on it, don't you plan to remove them?

robinmoussu commented 4 years ago

After reading the Readme, I understand your point of vue, but I think that you should comment it either in the commit message or in the code itself (or moving the Readme to an earlier commit).

rui314 commented 4 years ago

I'll update commit messages, and this should be addressed there. As to robustness, chibicc focuses only on handling correct code correctly and doesn't care too much about incorrect inputs. I think that's the appropriate prioritization, as oftentimes writing more code for error handling is a distraction especially during the very early stage of software development.

robinmoussu commented 4 years ago

That's totally understandable. You should also write it clearly. And probably adding a comment on places where a check isn't done correctly. Trying to re-write the first commits in Rust made it really obvious that bad things where going on, even if at first glance the C looks fine. I absolutely agree that error handling would harm comprehension, but I think that adding a comment would help the reader understand why this code isn't production ready (and never will, it's not its goal).