[clang] [Clang][Comments] Attach comments to decl even if preproc directives are in between (PR #88367)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 11 02:04:51 PDT 2024
https://github.com/hdoc created https://github.com/llvm/llvm-project/pull/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:
- Magnum: https://github.com/mosra/magnum/blob/8538610fa27e1db37070eaabe34f1e4e41648bab/src/Magnum/Resource.h#L117-L127
- libcds: https://github.com/khizmax/libcds/blob/9985d2a87feaa3e92532e28f4ab762a82855a49c/cds/container/michael_list_nogc.h#L36-L54
- 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](https://github.com/llvm/llvm-project/commit/b534d3a0ef6970f5e42f10ba5cfcb562d8b184e1)) 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](https://github.com/llvm/llvm-project/issues/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.
>From 54c114738a063d18aadedf470f005c3e966bd6e5 Mon Sep 17 00:00:00 2001
From: hdoc <github at hdoc.io>
Date: Thu, 11 Apr 2024 01:54:18 -0700
Subject: [PATCH] Attach comments to decl even if preproc directives are in
between
---
clang/lib/AST/ASTContext.cpp | 7 +-
clang/lib/Headers/amxcomplexintrin.h | 4 +
clang/test/Index/annotate-comments.cpp | 5 +-
clang/unittests/AST/DeclTest.cpp | 310 +++++++++++++++++++++++++
4 files changed, 321 insertions(+), 5 deletions(-)
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 2fa6aedca4c6ab..1c8d55b8627452 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 84ef972fcadf03..19eaee10ede659 100644
--- a/clang/lib/Headers/amxcomplexintrin.h
+++ b/clang/lib/Headers/amxcomplexintrin.h
@@ -62,6 +62,8 @@
/// The 2nd source tile. Max size is 1024 Bytes.
#define _tile_cmmimfp16ps(dst, a, b) __builtin_ia32_tcmmimfp16ps(dst, a, b)
+;
+
/// Perform matrix multiplication of two tiles containing complex elements and
/// accumulate the results into a packed single precision tile. Each dword
/// element in input tiles \a a and \a b is interpreted as a complex number
@@ -107,6 +109,8 @@
/// The 2nd source tile. Max size is 1024 Bytes.
#define _tile_cmmrlfp16ps(dst, a, b) __builtin_ia32_tcmmrlfp16ps(dst, a, b)
+;
+
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/test/Index/annotate-comments.cpp b/clang/test/Index/annotate-comments.cpp
index 6f9f8f0bbbc9e5..bff25d46cf80ee 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 2530ce74eb6a37..e5c1a578864292 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);
+ }
+}
More information about the cfe-commits
mailing list