<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 01/08/2013 10:36 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0KhCP1iWcUL7Pd4pbrawX_HYKbkLKxoHXyuCAEuo8nFcDQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>On Mon, Jan 7, 2013 at 10:56 AM, Shuxin Yang <span
dir="ltr"><<a moz-do-not-send="true"
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>
</div>
</div>
</blockquote>
Chandler, I did not "ignore" code reviewers' comment. In the
contrary, <br>
I strictly follow (almost) every single comment reviewers' figured
out. <br>
However, I had hard time in following your comment, something like
"what if the integer has 24 bit"?<br>
<br>
<blockquote
cite="mid:CAGCO0KhCP1iWcUL7Pd4pbrawX_HYKbkLKxoHXyuCAEuo8nFcDQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</div>
</blockquote>
By "focusing on the correctness part", I mean I ignore your comments
reading the potential correctness problem. <br>
As far as I can tell, they are absolute not a problem. <br>
About 80% of comments are based on your misunderstanding of
population-count and the code. <br>
<br>
<blockquote
cite="mid:CAGCO0KhCP1iWcUL7Pd4pbrawX_HYKbkLKxoHXyuCAEuo8nFcDQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div class="gmail_quote">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.<br>
</div>
</div>
</div>
</div>
</blockquote>
Basically, I'm open to any design pattern, as I said before in
engineering world, there is no absolute true of false.<br>
In your previous mail, you figure out some design issue, how come we
need to a helper class? <br>
How come the class need "Ncl_" (std for non-countable loop) prefix.
I believe these issues are <br>
really in a gray area. Like the 2nd issue. If I don't use "Ncl_"
prefix, how can I differentiate the <br>
patterns in countable loop and in non-countable loop? Ok, we can
certainly replace the prefix <br>
with other fancy names, will it make things tons better? <br>
<br>
<blockquote
cite="mid:CAGCO0KhCP1iWcUL7Pd4pbrawX_HYKbkLKxoHXyuCAEuo8nFcDQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
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>
</div>
</div>
</div>
</blockquote>
You don't have to debug. You just need to report the problem with a
testing cases. <br>
Isn't it a standard practice?<br>
<br>
Unfortunately, you didn't follow this practice, you believe you find
all the problems. You didn't. <br>
Since I didn't have testing case to verify, I miss the 2nd problem,
and piss you off. <br>
You promptly revert my changes...<br>
<br>
Now that it was my problem, I have to accept it, and I don't want to
retaliate either:-)<br>
<br>
But, I hope you will be nice to other folks. Has anybody reverted
your change to SROA,<br>
which has been buggy for a awful long time? What's point to make
the community so hostile?<br>
<br>
<blockquote
cite="mid:CAGCO0KhCP1iWcUL7Pd4pbrawX_HYKbkLKxoHXyuCAEuo8nFcDQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div class="gmail_quote">
<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>
</div>
</div>
</blockquote>
<blockquote
cite="mid:CAGCO0KhCP1iWcUL7Pd4pbrawX_HYKbkLKxoHXyuCAEuo8nFcDQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote"><br>
</div>
<div class="gmail_quote">[1]: <a moz-do-not-send="true"
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>
</div>
</blockquote>
<br>
Ok, I will fix this style problem, <br>
and I remember there are couple of space I need to remove.<br>
I will fix these problem in the following few days, maybe tomorrow.
<br>
<br>
<br>
<br>
</body>
</html>