<div dir="ltr"><div>On Mon, Jan 7, 2013 at 10:56 AM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank" class="cremed">shuxin.llvm@gmail.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Hi, Michael:<br>
<br>
Thanks! I actually have that mail and I also replied 50% of
his mails, and I also fixed one typo. <br>
I ignore the rest 50% of his mail because I don't think he
understand population-count.</div></div></blockquote><div><br></div><div>Shuxin, I know that you are new to LLVM project (and goodness knows we're a strange crew), but ignoring code review because you don't think the code reviewers understand your code isn't really a viable strategy.</div>
<div><br></div><div>First off, code reviews are critically important to the long-term health of the project. As part of that, it is important to cultivate, encourage, and help code reviewers who don't understand your code gain an understanding and become more involved in the project. In fact, that is part of being part of an open source project: helping others become more active and involved. If someone doesn't understand the code you're working on, you should explain it, or if you can re-direct them to web pages, articles, and books on the subject so they can learn and give increasingly relevant code review.</div>
<div><br></div><div>Secondly, I have some understanding population-count, so I don't think my understanding is really the issue at hand. </div><div><br>
</div><div>I haven't responded to your original email for two reasons:</div><div>1) You hadn't actually addressed or responded to the other half of my comments. I had assumed you were working on the actual bugs, and didn't want to divert the discussion until the bugs were fixed. Perhaps I should have directly replied.</div>
<div>2) Many of your responses simply state that you disagree and are, frankly, dismissive. Whether you agree or not, you cannot simply dismiss code review comments. If you disagree strongly, explain why and try to figure out a solution both you and your reviewer are happy with. If you don't disagree strongly, consider just making the change--it's a nice way to show you appreciate them taking the time to review your code. At the end of the day, when two contributors disagree strongly on an issue, we usually try to get a reasonable poll of different contributors' opinions and if necessary weight them by their contributions to the project.</div>
<div><div><br>Nevertheless, I will respond to a couple of the actual questions you raised since that seems to be what you are waiting for.</div><div><br></div><div>A few more comments on the rest of your email:</div>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
<div>I only focused on the correctness part.</div></div></blockquote><div><br></div><div>This simply isn't acceptable in the LLVM community. Code review, *all* of it, is critical. Coding style is part of it, but so is design review. I had comments and suggestions in both areas which have not been addressed, and consequentially maintaining and extending this code is harder for every other contributor.</div>
<div><br></div><div>If either style or design patterns don't make sense to you, I'm happy to discuss it further, but I will need you to help me understand the part that is confusing or unclear. I would also encourage you to look at some of the other code within the LLVM codebase, some of the commits of long-time contributors such as Chris, Dan, Duncan, or others. These might help you understand some of design and style principles of the project.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div>
As far as I can tell from
the assertion, I believe the <br>
case I miscompiled is a real pop-count -- it does count
population in the mean time <br>
do something else. Unfortunately, Chanlder was reluctant to show
me the minimal testing case, <br>
and only clam "it is not pop-count, but looks similar to it".
Actually, my implement had <br>
another problem he didn't figure out. <br></div></div></blockquote><div><br></div><div>I had very little time when debugging this. Sorry I didn't give you a complete test case. However, I was able to point at the exact bug in question as well as how to fix it, which should have helped you both fix and test the fix. You also would likely have written more popcnt test cases than I and might be able to do so more easily.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div>
Among all the stylistic issues, one issue I'm not quite sure. It
was about enum definition<br>
I copy the style from somewhere else in LLVM, and I check the
coding style std, I don't find <br>
any words about it. If you find a canonical way to define enum,
please let me know.</div></div></blockquote><div><br></div><div>From the coding standards document[1]:</div>"Unless the enumerators are defined in their own small namespace or inside a class, enumerators should have a prefix corresponding to the enum declaration name. For example, enum ValueKind { ... }; may contain enumerators like VK_Argument, VK_BasicBlock, etc."</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">[1]: <a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" target="_blank" class="cremed">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a></div>
<div class="gmail_quote"><br></div><div class="gmail_quote">As it happens, I've already cleaned up the enum while doing various other maintenance.</div></div></div>