Page MenuHomePhabricator

Adding doxygen comments to the LLVM intrinsics (part 6, popcntintrin.h)
ClosedPublic

Authored by kromanova on Feb 23 2016, 12:48 PM.

Details

Summary

Here is the patch with the doxygen comments for the intrinsincs in the header file popcntintrin.h.
The doxygen comments are automatically generated based on SCE internal intrinsics document using DCG tool that I wrote.

I will submit more doxygen comments for the other intrinsic header files as soon as this patch is approved.

Here is the link to the general discussion about adding comments to x86 intrinsics headers.
http://permalink.gmane.org/gmane.comp.compilers.clang.devel/42032

Here are the links to the similar code reviews for the doxygen comments in some other header files.

http://reviews.llvm.org/D8762 (closed)
http://reviews.llvm.org/D15999 (closed)
http://reviews.llvm.org/D16562 (closed)
http://reviews.llvm.org/D16913 (closed)
http://reviews.llvm.org/D17021 (closed)

Diff Detail

Repository
rL LLVM

Event Timeline

kromanova updated this revision to Diff 48844.Feb 23 2016, 12:48 PM
kromanova retitled this revision from to Adding doxygen comments to the LLVM intrinsics (part 6, popcntintrin.h).
kromanova updated this object.
kromanova set the repository for this revision to rL LLVM.
kromanova added a subscriber: cfe-commits.
probinson accepted this revision.Feb 23 2016, 4:30 PM
probinson edited edge metadata.

LGTM.

One question I have, which shouldn't block this (as we've done several like this already):
Is is okay to be using C++ style comments in these headers?
(Is there a C-style comment that Doxygen recognizes?)

This revision is now accepted and ready to land.Feb 23 2016, 4:30 PM
gbedwell added a subscriber: gbedwell.EditedFeb 23 2016, 10:34 PM

One question I have, which shouldn't block this (as we've done several like this already):
Is is okay to be using C++ style comments in these headers?
(Is there a C-style comment that Doxygen recognizes?)

There are a few various formats that Doxygen supports. Looking at headers from llvm-c the most common convention appears to be JavaDoc style, although there are a few examples of other supported styles floating around the codebase. E.g. from include/llvm-c/lto.h using JavaDoc style:

/**
 * Diagnostic handler type.
 * \p severity defines the severity.
 * \p diag is the actual diagnostic.
 * The diagnostic is not prefixed by any of severity keyword, e.g., 'error: '.
 * \p ctxt is used to pass the context set with the diagnostic handler.
 *
 * \since LTO_API_VERSION=7
 */

-Greg

echristo edited edge metadata.Feb 23 2016, 10:51 PM
echristo added a subscriber: echristo.

Yeah, we should be doing this. Nice catch Paul and Greg.

Hello,

I don’t think it will too hard to convert C++ style doxygen comments into C style doxygen comments by writing a post-processing python script. However, at first we need to decide if we really want to do that. If so, we need to settle on the exact format. After that, I need to make sure that the comments in the new format will be rendered correctly in MS Tooltips, XCode, online documentation and PS4 internal documentation. This discussion + investigation might take a few days.

Before we start discussing the exact format, I want to make sure that we really want to change to C-style doxygen comments.
Here are my not-so-strong arguments against it:

  • There currently are 257 occurrences C++ style comments in 14 other header files in /llvm/tools/clang/lib/Headers directory (I’m talking about the files that I didn’t touch). C++ style comments were there for AGES and nobody complained so far. If we decide to change C++ style doxygen comments -> C-style, we also need to change all regular C++ comments to C-style in these header files.
  • c99 (and later) supports C++ style comments, while I c89 doesn’t. I’m not sure if we have users that still use c89 format and x86 intrinsic headers at the same time.
  • C++ style doxygen comments are more pretty and readable compared to C-style comment (though it might be my subjective opinion).

Let me know what you think.

I will try to get Dmitri Gribenko’s opinion. He did a lot of work on doxygen in LLVM. I’m curious what he thinks about Javadoc style format.

Katya.

From: Eric Christopher [mailto:echristo@gmail.com]
Sent: Tuesday, February 23, 2016 10:51 PM
To: reviews+D17550+public+bc8ce213fd9db980@reviews.llvm.org; Romanova, Katya; Gao, Yunzhong; gribozavr@gmail.com; craig.topper@gmail.com; Robinson, Paul
Cc: Bedwell, Greg; cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D17550: Adding doxygen comments to the LLVM intrinsics (part 6, popcntintrin.h)

Yeah, we should be doing this. Nice catch Paul and Greg.

Those are all compelling reasons for me. Let's go with whatever you and
Dmitri think would be best for now. :)

-eric

Hi Dmitri,
Could you please let us know your opinion about C++ vs C-style doxygen comments. Read this thread for ‘pro’ and ‘con’ arguments about using C++ headers. Will LLVM online documentation look proper if we decide to use C-style headers? Which style do you personally prefer to see?

Note, that if there is any complaint in the future, it will take a couple of hours to write a python script to convert from C++ to C style doxygen comments.

Katya.

From: Eric Christopher [mailto:echristo@gmail.com]
Sent: Wednesday, February 24, 2016 6:40 PM
To: Romanova, Katya; reviews+D17550+public+bc8ce213fd9db980@reviews.llvm.org; Gao, Yunzhong; gribozavr@gmail.com; craig.topper@gmail.com; Robinson, Paul
Cc: Bedwell, Greg; cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D17550: Adding doxygen comments to the LLVM intrinsics (part 6, popcntintrin.h)

Those are all compelling reasons for me. Let's go with whatever you and Dmitri think would be best for now. :)

-eric

This revision was automatically updated to reflect the committed changes.