[llvm-commits] [PATCH] Population-count loop idiom recognization

[Sean Silva]

> +  /// CreateCtpop - Create and insert ctpop.
> +  CallInst *CreateCtpop(Value *Val);
>
> Please don't duplicate function or class name inside the comment.  Old
> code is written that way, but current coding style advises not to
> duplicate.

This link may be helpful for Shuxin:
<http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments>.

> +  class LIRUtil {
> +  public:
>
> This class has no data members, and all member functions are static.
> It makes sense to just put these functions in the anonymous namespace.

This actually contradicts the coding standards [1], which recommends
that free functions be marked as static instead of being put in an
anonymous namespace (i.e., only use anonymous namespaces for classes).
The rationale is that `static` provides a locality benefit to the
reader, since the fact that it is internal is immediately apparent
from the function declaration/definition, rather than having to search
for an enclosing anonymous namespace.

[1]. <http://llvm.org/docs/CodingStandards.html#anonymous-namespaces>

> This class has no data members, and all member functions are static.
> It makes sense to just put these functions in the anonymous namespace.
>
> +    static BranchInst *getBranch(BasicBlock * BB) {
>
> No space before '*', please.

I think you mean "after" (i.e. it should be `BasicBlock *BB`).

-- Sean Silva

On Mon, Nov 19, 2012 at 4:28 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
> Hi Shuxin,
>
> Just mechanical issues (I'm not familiar with this part of LLVM):
>
> +  /// CreateCtpop - Create and insert ctpop.
> +  CallInst *CreateCtpop(Value *Val);
>
> Please don't duplicate function or class name inside the comment.  Old
> code is written that way, but current coding style advises not to
> duplicate.
>
> +  assert(!(TyWidth & (TyWidth - 1)) && "Ty width must be power of 2");
>
> Please call isPowerOf2_32 from llvm/Support/MathExtras.h.
>
> +CallInst *IRBuilderBase::
> +CreateCtpop(Value *Val) {
>
> No newline after '::' needed.
>
> +  class LIRUtil {
> +  public:
>
> This class has no data members, and all member functions are static.
> It makes sense to just put these functions in the anonymous namespace.
>
> +    static BranchInst *getBranch(BasicBlock * BB) {
>
> No space before '*', please.
>
> Dmitri
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits