Hey jiang, Which commits would you like reviewed? I presume that some of these are already merged into mob, so if you just give the date of when you started your latest work, that would be sufficient.
For code review sorts of things, it's simplest if you work with feature branches. You can read about a use of feature branches for development in the "wild" <http://nvie.com/posts/a-successful-git-branching-model/>, as well as git's own documentation on branching work flows <http://git-scm.com/book/en/Git-Branching-Branching-Workflows>. Again, it's not necessary to work with feature branches, but it may prove helpful for code review. Looking at the most recent commit, here are my remarks: 1. Your commit message does not describe what your commit achieves. Why are you adding asserts? What are you asserting? See http://ablogaboutcode.com/2011/03/23/proper-git-commit-messages-and-an-elegant-git-history/ for guidance on writing a useful git commit message. 2. I am not sure that adding the asserts you add is appropriate. My point is not that this sort of defensive programming is bad. Rather, it seems to me like the addition of assertions makes the code more complex and probably a little slower. I believe that you could you achieve the same sort of debug checking by adding a test to the test suite, and would prefer to see that instead. 3. The TCC codebase uses the ST_FUNC attribute for a reason. Understand what it means and why it is used, and then use it. *Do not* rewrite a function and change its attributes in this way. 4. You have a pair of lines that say "if(p->r & VT_TMP) continue;" This alone could serve as a nice, atomic commit. Why did you add this? Does it improve the performance of the compiler? Do you actually have benchmarks that demonstrate a noticeable improvement, or are you adding lines of code with no measurable benefit? 5. White space changes should be in a separate commit. Better yet, don't make them unless they are actually functional and useful. (Hint: in C, white space changes are never functional and only very rarely useful. In other words, don't make them.) Those are my comments for your last push to that repo. In general I would not want to see this patch rolled into tcc. I think that assertions are a poor man's test suite. As we already have a test suite, these sorts of checks should go into the test suite, not the codebase itself. Hope that helps! David On Sun, Jun 1, 2014 at 12:47 PM, jiang <[email protected]> wrote: > https://gitcafe.com/weixiao/tinycc > > jiang > > _______________________________________________ > Tinycc-devel mailing list > [email protected] > https://lists.nongnu.org/mailman/listinfo/tinycc-devel > -- "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." -- Brian Kernighan
_______________________________________________ Tinycc-devel mailing list [email protected] https://lists.nongnu.org/mailman/listinfo/tinycc-devel
