[llvm-commits] [llvm] r168931 - in /llvm/trunk: include/llvm/Target/TargetTransformImpl.h include/llvm/TargetTransformInfo.h lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86ISelLowering.h lib/Target/X86/X86TargetMachine.h lib/Transforms/Scalar/LoopIdiomRecognize.cpp test/Transforms/LoopIdiom/popcnt.ll

[Chandler Carruth]

On Mon, Jan 7, 2013 at 10:56 AM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:

>  Hi, Michael:
>
>    Thanks!  I actually have that mail and I also replied 50% of his mails,
> and I also fixed one typo.
> I ignore the rest 50% of his mail because I don't think he understand
> population-count.
>

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.

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.

Secondly, I have some understanding population-count, so I don't think my
understanding is really the issue at hand.

I haven't responded to your original email for two reasons:
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.
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.

Nevertheless, I will respond to a couple of the actual questions you raised
since that seems to be what you are waiting for.

A few more comments on the rest of your email:

I only focused on the correctness part.
>

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.

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.

As far as I can tell from the assertion, I believe the
> case I miscompiled is a real pop-count  -- it does count population in the
> mean time
> do something else. Unfortunately, Chanlder was reluctant to show me the
> minimal testing case,
> and only clam "it is not pop-count, but looks similar to it".   Actually,
> my implement had
> another problem he didn't figure out.
>

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.

Among all the stylistic issues, one issue I'm not quite sure. It was about
> enum definition
> I copy the style from somewhere else in LLVM, and I check the coding style
> std, I don't find
> any words about it.  If you find a canonical way to define enum, please
> let me know.
>

>From the coding standards document[1]:
"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."

[1]:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

As it happens, I've already cleaned up the enum while doing various other
maintenance.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130108/4dd9abc4/attachment.html>