Skip to content

Commit 9f04d75

Browse files
authored
[Clang][Comments] Attach comments to decl even if preproc directives are in between (#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](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](#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.
1 parent a3f700a commit 9f04d75

File tree

6 files changed

+358
-5
lines changed

6 files changed

+358
-5
lines changed

clang/docs/ReleaseNotes.rst

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,22 @@ Clang Frontend Potentially Breaking Changes
152152
- The ``hasTypeLoc`` AST matcher will no longer match a ``classTemplateSpecializationDecl``;
153153
existing uses should switch to ``templateArgumentLoc`` or ``hasAnyTemplateArgumentLoc`` instead.
154154

155+
- The comment parser now matches comments to declarations even if there is a
156+
preprocessor macro in between the comment and declaration. This change is
157+
intended to improve Clang's support for parsing documentation comments and
158+
to better conform to Doxygen's behavior.
159+
160+
This has the potential to cause ``-Wdocumentation`` warnings, especially in
161+
cases where a function-like macro has a documentation comment and is followed
162+
immediately by a normal function. The function-like macro's documentation
163+
comments will be attributed to the subsequent function and may cause
164+
``-Wdocumentation`` warnings such as mismatched parameter names, or invalid
165+
return documentation comments.
166+
167+
In cases where the ``-Wdocumentation`` warnings are thrown, the suggested fix
168+
is to document the declaration following the macro so that the warnings are
169+
fixed.
170+
155171
Clang Python Bindings Potentially Breaking Changes
156172
--------------------------------------------------
157173
- Renamed ``CursorKind`` variant 272 from ``OMP_TEAMS_DISTRIBUTE_DIRECTIVE``

clang/lib/AST/ASTContext.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -284,9 +284,10 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
284284
StringRef Text(Buffer + CommentEndOffset,
285285
DeclLocDecomp.second - CommentEndOffset);
286286

287-
// There should be no other declarations or preprocessor directives between
288-
// comment and declaration.
289-
if (Text.find_last_of(";{}#@") != StringRef::npos)
287+
// There should be no other declarations between comment and declaration.
288+
// Preprocessor directives are implicitly allowed to be between a comment and
289+
// its associated decl.
290+
if (Text.find_last_of(";{}@") != StringRef::npos)
290291
return nullptr;
291292

292293
return CommentBeforeDecl;

clang/lib/Headers/amxcomplexintrin.h

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,24 @@
107107
/// The 2nd source tile. Max size is 1024 Bytes.
108108
#define _tile_cmmrlfp16ps(dst, a, b) __builtin_ia32_tcmmrlfp16ps(dst, a, b)
109109

110+
/// Perform matrix multiplication of two tiles containing complex elements and
111+
/// accumulate the results into a packed single precision tile.
112+
///
113+
/// \param m
114+
/// The number of rows in the first tile and the number of rows in the result
115+
/// tile.
116+
/// \param n
117+
/// The number of columns in the second tile and the number of columns in the
118+
/// result tile.
119+
/// \param k
120+
/// The number of columns in the first tile and the number of rows in the
121+
/// second tile.
122+
/// \param dst
123+
/// Pointer to the destination tile where the result will be stored.
124+
/// \param src1
125+
/// Pointer to the first source tile.
126+
/// \param src2
127+
/// Pointer to the second source tile.
110128
static __inline__ _tile1024i __DEFAULT_FN_ATTRS_COMPLEX
111129
_tile_cmmimfp16ps_internal(unsigned short m, unsigned short n, unsigned short k,
112130
_tile1024i dst, _tile1024i src1, _tile1024i src2) {

clang/lib/Headers/ia32intrin.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,13 @@ __rdtscp(unsigned int *__A) {
533533
/// \see __rdpmc
534534
#define _rdpmc(A) __rdpmc(A)
535535

536+
/// Invalidates the contents of the processor's internal caches.
537+
/// This function writes back and invalidates all modified cache lines in
538+
/// the processor.
539+
///
540+
/// \headerfile <x86intrin.h>
541+
///
542+
/// This intrinsic corresponds to the \c WBINVD instruction.
536543
static __inline__ void __DEFAULT_FN_ATTRS
537544
_wbinvd(void) {
538545
__builtin_ia32_wbinvd();

clang/test/Index/annotate-comments.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -204,9 +204,9 @@ void isdoxy45(void);
204204
/// Ggg. IS_DOXYGEN_END
205205
void isdoxy46(void);
206206

207-
/// IS_DOXYGEN_NOT_ATTACHED
207+
/// isdoxy47 IS_DOXYGEN_SINGLE
208208
#define FOO
209-
void notdoxy47(void);
209+
void isdoxy47(void);
210210

211211
/// IS_DOXYGEN_START Aaa bbb
212212
/// \param ccc
@@ -330,6 +330,7 @@ void isdoxy54(int);
330330
// CHECK: annotate-comments.cpp:185:6: FunctionDecl=isdoxy44:{{.*}} BriefComment=[IS_DOXYGEN_START Aaa bbb ccc.]
331331
// CHECK: annotate-comments.cpp:195:6: FunctionDecl=isdoxy45:{{.*}} BriefComment=[Ddd eee. Fff.]
332332
// CHECK: annotate-comments.cpp:205:6: FunctionDecl=isdoxy46:{{.*}} BriefComment=[Ddd eee. Fff.]
333+
// CHECK: annotate-comments.cpp:209:6: FunctionDecl=isdoxy47:{{.*}} isdoxy47 IS_DOXYGEN_SINGLE
333334
// CHECK: annotate-comments.cpp:214:6: FunctionDecl=isdoxy48:{{.*}} BriefComment=[IS_DOXYGEN_START Aaa bbb]
334335
// CHECK: annotate-comments.cpp:218:6: FunctionDecl=isdoxy49:{{.*}} BriefComment=[IS_DOXYGEN_START Aaa]
335336
// CHECK: annotate-comments.cpp:222:6: FunctionDecl=isdoxy50:{{.*}} BriefComment=[Returns ddd IS_DOXYGEN_END]

0 commit comments

Comments
 (0)