[Clang][Comments] Attach comments to decl even if preproc directives are in between #88367
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
hdoc
commented
Apr 11, 2024
hdoc
force-pushed
the
dev/attach-comment-to-decl
branch
from
54c1147 to
61612c5
Compare
|
Ping |
|
Regarding the positioning of Doxygen comments, I just saw this and was wondering if it's possible to also support parsing of Doxygen comments that come after the signature, which happens to be the preferred style in our codebase ... static int doSomething(int foobar)
/** \brief ...
* \param foobar foo bar
* \return return value
*/
{
...Doxygen accepts this, although I don't know if it's specified/documented as such. |
|
From my reading of the Doxygen documentation it seems like anything goes except putting the doc comment inside the actual body of a function. That being said, I think that change is out of the scope of this PR because it would likely require modifications to the Clang comment-to-decl matching functionality. It's better to do that in a separate PR |
|
Is this still in Draft status or should this be marked as ready for review? |
|
@AaronBallman this one is ready for review :) |
llvmbot
added
clang:frontend
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-clang Author: hdoc (hdoc) ChangesBackgroundIt's surprisingly common for C++ code in the wild to conditionally show/hide declarations to Doxygen through the use of preprocessor directives. One especially common version of this pattern is demonstrated below: /// @<!-- -->brief Test comment
#ifdef DOXYGEN_BUILD_ENABLED
template<typename T>
#else
template <typename T>
typename std::enable_if<std::is_integral<T>::value>::type
#endif
void f() {}There are more examples I've collected below to demonstrate usage of this pattern: From my research, it seems like the most common rationale for this functionality is hiding difficult-to-parse code from Doxygen, especially where template metaprogramming is concerned. Currently, Clang does not support attaching comments to decls if there are preprocessor comments between the comment and the decl. This is enforced here: llvm-project/clang/lib/AST/ASTContext.cpp Lines 284 to 287 in b6ebea7 Alongside preprocessor directives, any instance of RationaleIt would be nice for Clang-based documentation tools, such as hdoc, to support code using this pattern. Users expect to see comments attached to the relevant decl — even if there is an HistoryOriginally, commas were also in the list of "banned" characters, but were removed in ChangeThis modifies Clang comment parsing so that comments are attached to subsequent declarations even if there are preprocessor directives between the end of the comment and the start of the decl. Furthermore, this change:
ComplicationsClang does not yet support attaching doc comments to macros. Consequently, the change proposed in this RFC affects cases where a doc comment attached to a macro is followed immediately by a normal declaration. In these cases, the macro's doc comments will be attached to the subsequent decl. Previously they would be ignored because any preprocessor directives between a comment and a decl would result in the comment not being attached to the decl. An example of this is shown below. /// Doc comment for a function-like macro
/// @<!-- -->param n
/// A macro argument
#define custom_sqrt(n) __internal_sqrt(n)
int __internal_sqrt(int n) { return __builtin_sqrt(n); }
// NB: the doc comment for the custom_sqrt macro will actually be attached to __internal_sqrt!There is a real instance of this problem in the Clang codebase, namely here: llvm-project/clang/lib/Headers/amxcomplexintrin.h Lines 65 to 114 in be10070 As part of this RFC, I've added a semicolon to break up the Clang comment parsing so that the Full diff: https://github.com/llvm/llvm-project/pull/88367.diff 5 Files Affected:
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 2fa6aedca4c6..1c8d55b86274 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -282,9 +282,10 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
StringRef Text(Buffer + CommentEndOffset,
DeclLocDecomp.second - CommentEndOffset);
- // There should be no other declarations or preprocessor directives between
- // comment and declaration.
- if (Text.find_last_of(";{}#@") != StringRef::npos)
+ // There should be no other declarations between comment and declaration.
+ // Preprocessor directives are implicitly allowed to be between a comment and
+ // its associated decl.
+ if (Text.find_last_of(";{}@") != StringRef::npos)
return nullptr;
return CommentBeforeDecl;
diff --git a/clang/lib/Headers/amxcomplexintrin.h b/clang/lib/Headers/amxcomplexintrin.h
index 84ef972fcadf..6ae40d2eab5c 100644
--- a/clang/lib/Headers/amxcomplexintrin.h
+++ b/clang/lib/Headers/amxcomplexintrin.h
@@ -107,6 +107,24 @@
/// The 2nd source tile. Max size is 1024 Bytes.
#define _tile_cmmrlfp16ps(dst, a, b) __builtin_ia32_tcmmrlfp16ps(dst, a, b)
+/// Perform matrix multiplication of two tiles containing complex elements and
+/// accumulate the results into a packed single precision tile.
+///
+/// \param m
+/// The number of rows in the first tile and the number of rows in the result
+/// tile.
+/// \param n
+/// The number of columns in the second tile and the number of columns in the
+/// result tile.
+/// \param k
+/// The number of columns in the first tile and the number of rows in the
+/// second tile.
+/// \param dst
+/// Pointer to the destination tile where the result will be stored.
+/// \param src1
+/// Pointer to the first source tile.
+/// \param src2
+/// Pointer to the second source tile.
static __inline__ _tile1024i __DEFAULT_FN_ATTRS_COMPLEX
_tile_cmmimfp16ps_internal(unsigned short m, unsigned short n, unsigned short k,
_tile1024i dst, _tile1024i src1, _tile1024i src2) {
diff --git a/clang/lib/Headers/ia32intrin.h b/clang/lib/Headers/ia32intrin.h
index 8e65f232a0de..c9c92b9ee0f9 100644
--- a/clang/lib/Headers/ia32intrin.h
+++ b/clang/lib/Headers/ia32intrin.h
@@ -533,6 +533,8 @@ __rdtscp(unsigned int *__A) {
/// \see __rdpmc
#define _rdpmc(A) __rdpmc(A)
+;
+
static __inline__ void __DEFAULT_FN_ATTRS
_wbinvd(void) {
__builtin_ia32_wbinvd();
diff --git a/clang/test/Index/annotate-comments.cpp b/clang/test/Index/annotate-comments.cpp
index 6f9f8f0bbbc9..bff25d46cf80 100644
--- a/clang/test/Index/annotate-comments.cpp
+++ b/clang/test/Index/annotate-comments.cpp
@@ -204,9 +204,9 @@ void isdoxy45(void);
/// Ggg. IS_DOXYGEN_END
void isdoxy46(void);
-/// IS_DOXYGEN_NOT_ATTACHED
+/// isdoxy47 IS_DOXYGEN_SINGLE
#define FOO
-void notdoxy47(void);
+void isdoxy47(void);
/// IS_DOXYGEN_START Aaa bbb
/// \param ccc
@@ -330,6 +330,7 @@ void isdoxy54(int);
// CHECK: annotate-comments.cpp:185:6: FunctionDecl=isdoxy44:{{.*}} BriefComment=[IS_DOXYGEN_START Aaa bbb ccc.]
// CHECK: annotate-comments.cpp:195:6: FunctionDecl=isdoxy45:{{.*}} BriefComment=[Ddd eee. Fff.]
// CHECK: annotate-comments.cpp:205:6: FunctionDecl=isdoxy46:{{.*}} BriefComment=[Ddd eee. Fff.]
+// CHECK: annotate-comments.cpp:209:6: FunctionDecl=isdoxy47:{{.*}} isdoxy47 IS_DOXYGEN_SINGLE
// CHECK: annotate-comments.cpp:214:6: FunctionDecl=isdoxy48:{{.*}} BriefComment=[IS_DOXYGEN_START Aaa bbb]
// CHECK: annotate-comments.cpp:218:6: FunctionDecl=isdoxy49:{{.*}} BriefComment=[IS_DOXYGEN_START Aaa]
// CHECK: annotate-comments.cpp:222:6: FunctionDecl=isdoxy50:{{.*}} BriefComment=[Returns ddd IS_DOXYGEN_END]
diff --git a/clang/unittests/AST/DeclTest.cpp b/clang/unittests/AST/DeclTest.cpp
index 2530ce74eb6a..e5c1a5788642 100644
--- a/clang/unittests/AST/DeclTest.cpp
+++ b/clang/unittests/AST/DeclTest.cpp
@@ -545,3 +545,313 @@ TEST(Decl, TemplateArgumentDefaulted) {
EXPECT_TRUE(ArgList.get(2).getIsDefaulted());
EXPECT_TRUE(ArgList.get(3).getIsDefaulted());
}
+
+TEST(Decl, CommentsAttachedToDecl1) {
+ const SmallVector<StringRef> Sources{
+ R"(
+ /// Test comment
+ void f();
+ )",
+
+ R"(
+ /// Test comment
+
+ void f();
+ )",
+
+ R"(
+ /// Test comment
+ #if 0
+ // tralala
+ #endif
+ void f();
+ )",
+
+ R"(
+ /// Test comment
+
+ #if 0
+ // tralala
+ #endif
+
+ void f();
+ )",
+
+ R"(
+ /// Test comment
+ #ifdef DOCS
+ template<typename T>
+ #endif
+ void f();
+ )",
+
+ R"(
+ /// Test comment
+
+ #ifdef DOCS
+ template<typename T>
+ #endif
+
+ void f();
+ )",
+ };
+
+ for (const auto code : Sources) {
+ auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+ ASTContext &Ctx = AST->getASTContext();
+
+ auto const *F = selectFirst<FunctionDecl>(
+ "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+ ASSERT_NE(F, nullptr);
+
+ auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+ ASSERT_NE(C, nullptr);
+ EXPECT_EQ(C->getRawText(Ctx.getSourceManager()), "/// Test comment");
+ }
+}
+
+TEST(Decl, CommentsAttachedToDecl2) {
+ const SmallVector<StringRef> Sources{
+ R"(
+ /** Test comment
+ */
+ void f();
+ )",
+
+ R"(
+ /** Test comment
+ */
+
+ void f();
+ )",
+
+ R"(
+ /** Test comment
+ */
+ #if 0
+ /* tralala */
+ #endif
+ void f();
+ )",
+
+ R"(
+ /** Test comment
+ */
+
+ #if 0
+ /* tralala */
+ #endif
+
+ void f();
+ )",
+
+ R"(
+ /** Test comment
+ */
+ #ifdef DOCS
+ template<typename T>
+ #endif
+ void f();
+ )",
+
+ R"(
+ /** Test comment
+ */
+
+ #ifdef DOCS
+ template<typename T>
+ #endif
+
+ void f();
+ )",
+ };
+
+ for (const auto code : Sources) {
+ auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+ ASTContext &Ctx = AST->getASTContext();
+
+ auto const *F = selectFirst<FunctionDecl>(
+ "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+ ASSERT_NE(F, nullptr);
+
+ auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+ ASSERT_NE(C, nullptr);
+ EXPECT_EQ(C->getRawText(Ctx.getSourceManager()),
+ "/** Test comment\n */");
+ }
+}
+
+TEST(Decl, CommentsAttachedToDecl3) {
+ const SmallVector<StringRef> Sources{
+ R"(
+ /// @brief Test comment
+ void f();
+ )",
+
+ R"(
+ /// @brief Test comment
+
+ void f();
+ )",
+
+ R"(
+ /// @brief Test comment
+ #if 0
+ // tralala
+ #endif
+ void f();
+ )",
+
+ R"(
+ /// @brief Test comment
+
+ #if 0
+ // tralala
+ #endif
+
+ void f();
+ )",
+
+ R"(
+ /// @brief Test comment
+ #ifdef DOCS
+ template<typename T>
+ #endif
+ void f();
+ )",
+
+ R"(
+ /// @brief Test comment
+
+ #ifdef DOCS
+ template<typename T>
+ #endif
+
+ void f();
+ )",
+ };
+
+ for (const auto code : Sources) {
+ auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+ ASTContext &Ctx = AST->getASTContext();
+
+ auto const *F = selectFirst<FunctionDecl>(
+ "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+ ASSERT_NE(F, nullptr);
+
+ auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+ ASSERT_NE(C, nullptr);
+ EXPECT_EQ(C->getRawText(Ctx.getSourceManager()), "/// @brief Test comment");
+ }
+}
+
+TEST(Decl, CommentsAttachedToDecl4) {
+ const SmallVector<StringRef> Sources{
+ R"(
+ /** \brief Test comment
+ */
+ void f();
+ )",
+
+ R"(
+ /** \brief Test comment
+ */
+
+ void f();
+ )",
+
+ R"(
+ /** \brief Test comment
+ */
+ #if 0
+ /* tralala */
+ #endif
+ void f();
+ )",
+
+ R"(
+ /** \brief Test comment
+ */
+
+ #if 0
+ /* tralala */
+ #endif
+
+ void f();
+ )",
+
+ R"(
+ /** \brief Test comment
+ */
+ #ifdef DOCS
+ template<typename T>
+ #endif
+ void f();
+ )",
+
+ R"(
+ /** \brief Test comment
+ */
+
+ #ifdef DOCS
+ template<typename T>
+ #endif
+
+ void f();
+ )",
+ };
+
+ for (const auto code : Sources) {
+ auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+ ASTContext &Ctx = AST->getASTContext();
+
+ auto const *F = selectFirst<FunctionDecl>(
+ "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+ ASSERT_NE(F, nullptr);
+
+ auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+ ASSERT_NE(C, nullptr);
+ EXPECT_EQ(C->getRawText(Ctx.getSourceManager()),
+ "/** \\brief Test comment\n */");
+ }
+}
+
+/// This example intentionally inserts characters between a doc comment and the
+/// associated declaration to verify that the comment does not become associated
+/// with the FunctionDecl.
+/// By default, Clang does not allow for other declarations (aside from
+/// preprocessor directives, as shown above) to be placed between a doc comment
+/// and a declaration.
+TEST(Decl, CommentsAttachedToDecl5) {
+ const SmallVector<StringRef> Sources{
+ R"(
+ /// Test comment
+ ;
+ void f();
+ )",
+
+ R"(
+ /// Test comment
+ // @
+ void f();
+ )",
+
+ R"(
+ /// Test comment
+ // {}
+ void f();
+ )",
+ };
+
+ for (const auto code : Sources) {
+ auto AST = tooling::buildASTFromCodeWithArgs(code, /*Args=*/{"-std=c++20"});
+ ASTContext &Ctx = AST->getASTContext();
+
+ auto const *F = selectFirst<FunctionDecl>(
+ "id", match(functionDecl(hasName("f")).bind("id"), Ctx));
+ ASSERT_NE(F, nullptr);
+
+ auto const *C = Ctx.getRawCommentForDeclNoCache(F);
+ ASSERT_EQ(C, nullptr);
+ }
+}
|
There was a problem hiding this comment.
Because doxygen supports documenting macros (https://www.doxygen.nl/manual/commands.html#cmddef), I am worried how often this will cause us to associate comments incorrectly on the declaration.
I wonder if we should be a bit smarter and check for #define at the start of a line when we encounter a #. e.g.,
/*!
\def DERP(x)
Does a derpy thing with x
*/
#define DERP(x) (x)
void derp(void); // Does not get the doxygen comment
while
/// Does amazing things, like works in the presence of
/// #define doing stupid things.
void func(); // does get the doxygen comment
AaronBallman
reviewed
Jun 6, 2024
hdoc
force-pushed
the
dev/attach-comment-to-decl
branch
from
31c954f to
571fb55
Compare
|
@AaronBallman Added the blurb to the release notes, let me know if that matches your expectations or if any changes are required. Thank you! |
|
Ping |
The lit test compiles the code with -Wdocumentation set, so we need to document the internal function so that -Wdocumentation doesn't throw errors
hdoc
force-pushed
the
dev/attach-comment-to-decl
branch
from
823cd80 to
d42bb16
Compare
|
This is rebased onto the latest rev of the main branch and should be ready to go. We do not have commit permissions, so @AaronBallman could you please help merge this in? Thank you :) |
|
@hdoc Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/835 Here is the relevant piece of the build log for the reference: |
|
I'd like to push back against this change, I believe it is too broad. (Full disclosure: I'm the original author of this subsystem in Clang.) We (Google) are integrating this change downstream and it causes a bunch of comments to get associated to unrelated declarations. The problem is particularly bad with file-level comments, which get attached to the first declaration. For example: https://github.com/google/crubit/blob/main/rs_bindings_from_cc/test/golden/definition_of_forward_declaration.h In this file the license comment gets attached to I think you should at the very minimum check for |
|
|
|
+1 on the revert and thank you for letting me know about this. I'll make changes to limit how aggressively the frontend attaches comments to decls inline with what @gribozavr and @danakj suggested. |
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…are in between (llvm#88367) ### Background It's surprisingly common for C++ code in the wild to conditionally show/hide declarations to Doxygen through the use of preprocessor directives. One especially common version of this pattern is demonstrated below: ```cpp /// @brief Test comment #ifdef DOXYGEN_BUILD_ENABLED template<typename T> #else template <typename T> typename std::enable_if<std::is_integral<T>::value>::type #endif void f() {} ``` There are more examples I've collected below to demonstrate usage of this pattern: - Example 1: [Magnum](https://github.com/mosra/magnum/blob/8538610fa27e1db37070eaabe34f1e4e41648bab/src/Magnum/Resource.h#L117-L127) - Example 2: [libcds](https://github.com/khizmax/libcds/blob/9985d2a87feaa3e92532e28f4ab762a82855a49c/cds/container/michael_list_nogc.h#L36-L54) - Example 3: [rocPRIM](https://github.com/ROCm/rocPRIM/blob/609ae19565ff6a3499168b76a0be5652762e24f6/rocprim/include/rocprim/block/detail/block_reduce_raking_reduce.hpp#L60-L65) From my research, it seems like the most common rationale for this functionality is hiding difficult-to-parse code from Doxygen, especially where template metaprogramming is concerned. Currently, Clang does not support attaching comments to decls if there are preprocessor comments between the comment and the decl. This is enforced here: https://github.com/llvm/llvm-project/blob/b6ebea7972cd05a8e4dcf0d5a54f2c793999995a/clang/lib/AST/ASTContext.cpp#L284-L287 Alongside preprocessor directives, any instance of `;{}#@` between a comment and decl will cause the comment to not be attached to the decl. #### Rationale It would be nice for Clang-based documentation tools, such as [hdoc](https://hdoc.io), to support code using this pattern. Users expect to see comments attached to the relevant decl — even if there is an `#ifdef` in the way — which Clang does not currently do. #### History Originally, commas were also in the list of "banned" characters, but were removed in `b534d3a0ef69` ([link](llvm@b534d3a)) because availability macros often have commas in them. From my reading of the code, it appears that the original intent of the code was to exclude macros and decorators between comments and decls, possibly in an attempt to properly attribute comments to macros (discussed further in "Complications", below). There's some more discussion here: https://reviews.llvm.org/D125061. ### Change This modifies Clang comment parsing so that comments are attached to subsequent declarations even if there are preprocessor directives between the end of the comment and the start of the decl. Furthermore, this change: - Adds tests to verify that comments are attached to their associated decls even if there are preprocessor directives in between - Adds tests to verify that current behavior has not changed (i.e. use of the other characters between comment and decl will result in the comment not being attached to the decl) - Updates existing `lit` tests which would otherwise break. #### Complications Clang [does not yet support](llvm#38206) attaching doc comments to macros. Consequently, the change proposed in this RFC affects cases where a doc comment attached to a macro is followed immediately by a normal declaration. In these cases, the macro's doc comments will be attached to the subsequent decl. Previously they would be ignored because any preprocessor directives between a comment and a decl would result in the comment not being attached to the decl. An example of this is shown below. ```cpp /// Doc comment for a function-like macro /// @param n /// A macro argument #define custom_sqrt(n) __internal_sqrt(n) int __internal_sqrt(int n) { return __builtin_sqrt(n); } // NB: the doc comment for the custom_sqrt macro will actually be attached to __internal_sqrt! ``` There is a real instance of this problem in the Clang codebase, namely here: https://github.com/llvm/llvm-project/blob/be10070f91b86a6f126d2451852242bfcb2cd366/clang/lib/Headers/amxcomplexintrin.h#L65-L114 As part of this RFC, I've added a semicolon to break up the Clang comment parsing so that the `-Wdocumentation` errors go away, but this is a hack. The real solution is to fix Clang comment parsing so that doc comments are properly attached to macros, however this would be a large change that is outside of the scope of this RFC.
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…ectives are in between (llvm#88367)" This reverts commit 9f04d75. There was post-commit feedback on the direction this PR took.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…are in between (llvm#88367) ### Background It's surprisingly common for C++ code in the wild to conditionally show/hide declarations to Doxygen through the use of preprocessor directives. One especially common version of this pattern is demonstrated below: ```cpp /// @brief Test comment #ifdef DOXYGEN_BUILD_ENABLED template<typename T> #else template <typename T> typename std::enable_if<std::is_integral<T>::value>::type #endif void f() {} ``` There are more examples I've collected below to demonstrate usage of this pattern: - Example 1: [Magnum](https://github.com/mosra/magnum/blob/8538610fa27e1db37070eaabe34f1e4e41648bab/src/Magnum/Resource.h#L117-L127) - Example 2: [libcds](https://github.com/khizmax/libcds/blob/9985d2a87feaa3e92532e28f4ab762a82855a49c/cds/container/michael_list_nogc.h#L36-L54) - Example 3: [rocPRIM](https://github.com/ROCm/rocPRIM/blob/609ae19565ff6a3499168b76a0be5652762e24f6/rocprim/include/rocprim/block/detail/block_reduce_raking_reduce.hpp#L60-L65) From my research, it seems like the most common rationale for this functionality is hiding difficult-to-parse code from Doxygen, especially where template metaprogramming is concerned. Currently, Clang does not support attaching comments to decls if there are preprocessor comments between the comment and the decl. This is enforced here: https://github.com/llvm/llvm-project/blob/b6ebea7972cd05a8e4dcf0d5a54f2c793999995a/clang/lib/AST/ASTContext.cpp#L284-L287 Alongside preprocessor directives, any instance of `;{}#@` between a comment and decl will cause the comment to not be attached to the decl. #### Rationale It would be nice for Clang-based documentation tools, such as [hdoc](https://hdoc.io), to support code using this pattern. Users expect to see comments attached to the relevant decl — even if there is an `#ifdef` in the way — which Clang does not currently do. #### History Originally, commas were also in the list of "banned" characters, but were removed in `b534d3a0ef69` ([link](llvm@b534d3a)) because availability macros often have commas in them. From my reading of the code, it appears that the original intent of the code was to exclude macros and decorators between comments and decls, possibly in an attempt to properly attribute comments to macros (discussed further in "Complications", below). There's some more discussion here: https://reviews.llvm.org/D125061. ### Change This modifies Clang comment parsing so that comments are attached to subsequent declarations even if there are preprocessor directives between the end of the comment and the start of the decl. Furthermore, this change: - Adds tests to verify that comments are attached to their associated decls even if there are preprocessor directives in between - Adds tests to verify that current behavior has not changed (i.e. use of the other characters between comment and decl will result in the comment not being attached to the decl) - Updates existing `lit` tests which would otherwise break. #### Complications Clang [does not yet support](llvm#38206) attaching doc comments to macros. Consequently, the change proposed in this RFC affects cases where a doc comment attached to a macro is followed immediately by a normal declaration. In these cases, the macro's doc comments will be attached to the subsequent decl. Previously they would be ignored because any preprocessor directives between a comment and a decl would result in the comment not being attached to the decl. An example of this is shown below. ```cpp /// Doc comment for a function-like macro /// @param n /// A macro argument #define custom_sqrt(n) __internal_sqrt(n) int __internal_sqrt(int n) { return __builtin_sqrt(n); } // NB: the doc comment for the custom_sqrt macro will actually be attached to __internal_sqrt! ``` There is a real instance of this problem in the Clang codebase, namely here: https://github.com/llvm/llvm-project/blob/be10070f91b86a6f126d2451852242bfcb2cd366/clang/lib/Headers/amxcomplexintrin.h#L65-L114 As part of this RFC, I've added a semicolon to break up the Clang comment parsing so that the `-Wdocumentation` errors go away, but this is a hack. The real solution is to fix Clang comment parsing so that doc comments are properly attached to macros, however this would be a large change that is outside of the scope of this RFC.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…ectives are in between (llvm#88367)" This reverts commit 9f04d75. There was post-commit feedback on the direction this PR took.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
It's surprisingly common for C++ code in the wild to conditionally show/hide declarations to Doxygen through the use of preprocessor directives. One especially common version of this pattern is demonstrated below:
There are more examples I've collected below to demonstrate usage of this pattern:
From my research, it seems like the most common rationale for this functionality is hiding difficult-to-parse code from Doxygen, especially where template metaprogramming is concerned.
Currently, Clang does not support attaching comments to decls if there are preprocessor comments between the comment and the decl. This is enforced here:
llvm-project/clang/lib/AST/ASTContext.cpp
Lines 284 to 287 in b6ebea7
Alongside preprocessor directives, any instance of
;{}#@between a comment and decl will cause the comment to not be attached to the decl.Rationale
It would be nice for Clang-based documentation tools, such as hdoc, to support code using this pattern. Users expect to see comments attached to the relevant decl — even if there is an
#ifdefin the way — which Clang does not currently do.History
Originally, commas were also in the list of "banned" characters, but were removed in
b534d3a0ef69(link) because availability macros often have commas in them. From my reading of the code, it appears that the original intent of the code was to exclude macros and decorators between comments and decls, possibly in an attempt to properly attribute comments to macros (discussed further in "Complications", below). There's some more discussion here: https://reviews.llvm.org/D125061.Change
This modifies Clang comment parsing so that comments are attached to subsequent declarations even if there are preprocessor directives between the end of the comment and the start of the decl. Furthermore, this change:
littests which would otherwise break.Complications
Clang does not yet support attaching doc comments to macros. Consequently, the change proposed in this RFC affects cases where a doc comment attached to a macro is followed immediately by a normal declaration. In these cases, the macro's doc comments will be attached to the subsequent decl. Previously they would be ignored because any preprocessor directives between a comment and a decl would result in the comment not being attached to the decl. An example of this is shown below.
There is a real instance of this problem in the Clang codebase, namely here:
llvm-project/clang/lib/Headers/amxcomplexintrin.h
Lines 65 to 114 in be10070
As part of this RFC, I've added a semicolon to break up the Clang comment parsing so that the
-Wdocumentationerrors go away, but this is a hack. The real solution is to fix Clang comment parsing so that doc comments are properly attached to macros, however this would be a large change that is outside of the scope of this RFC.