[clang] [clang] Prioritze decl comments from macro expansion site (PR #65481)
Daniel Grumberg via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 23 06:38:50 PDT 2023
https://github.com/daniel-grumberg updated https://github.com/llvm/llvm-project/pull/65481
>From 32155e8b5ac01242c3e16968f9a7c821d16b7007 Mon Sep 17 00:00:00 2001
From: Daniel Grumberg <dgrumberg at apple.com>
Date: Wed, 6 Sep 2023 12:20:30 +0100
Subject: [PATCH 1/2] [clang] Prioritze decl comments from macro expansion site
For declarations declared inside a macro, e.g.:
```
`#define MAKE_FUNC(suffix) \
/// Not selected doc comment \
void func_##suffix(void) { }
/// Doc comment foo
MAKE_FUNC(foo)
/// Doc comment bar
MAKE_FUNC(bar)
````
Prefer the doc comment at the expansion site instead of the one defined
in the macro.
rdar://113995729
---
clang/lib/AST/ASTContext.cpp | 191 +++++++--------------
clang/test/Index/annotate-comments-objc.m | 12 +-
clang/unittests/Tooling/SourceCodeTest.cpp | 5 +-
3 files changed, 70 insertions(+), 138 deletions(-)
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 4b1d9e86797b778..074f59bd5a50411 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -112,10 +112,10 @@ enum FloatingRank {
Ibm128Rank
};
-/// \returns location that is relevant when searching for Doc comments related
-/// to \p D.
-static SourceLocation getDeclLocForCommentSearch(const Decl *D,
- SourceManager &SourceMgr) {
+/// \returns The locations that are relevant when searching for Doc comments
+/// related to \p D.
+static SmallVector<SourceLocation, 2>
+getDeclLocsForCommentSearch(const Decl *D, SourceManager &SourceMgr) {
assert(D);
// User can not attach documentation to implicit declarations.
@@ -167,115 +167,41 @@ static SourceLocation getDeclLocForCommentSearch(const Decl *D,
isa<TemplateTemplateParmDecl>(D))
return {};
+ SmallVector<SourceLocation, 2> Locations;
// Find declaration location.
// For Objective-C declarations we generally don't expect to have multiple
// declarators, thus use declaration starting location as the "declaration
// location".
// For all other declarations multiple declarators are used quite frequently,
// so we use the location of the identifier as the "declaration location".
+ SourceLocation BaseLocation;
if (isa<ObjCMethodDecl>(D) || isa<ObjCContainerDecl>(D) ||
- isa<ObjCPropertyDecl>(D) ||
- isa<RedeclarableTemplateDecl>(D) ||
+ isa<ObjCPropertyDecl>(D) || isa<RedeclarableTemplateDecl>(D) ||
isa<ClassTemplateSpecializationDecl>(D) ||
// Allow association with Y across {} in `typedef struct X {} Y`.
isa<TypedefDecl>(D))
- return D->getBeginLoc();
+ BaseLocation = D->getBeginLoc();
+ else
+ BaseLocation = D->getLocation();
- const SourceLocation DeclLoc = D->getLocation();
- if (DeclLoc.isMacroID()) {
- // There are (at least) three types of macros we care about here.
- //
- // 1. Macros that are used in the definition of a type outside the macro,
- // with a comment attached at the macro call site.
- // ```
- // #define MAKE_NAME(Foo) Name##Foo
- //
- // /// Comment is here, where we use the macro.
- // struct MAKE_NAME(Foo) {
- // int a;
- // int b;
- // };
- // ```
- // 2. Macros that define whole things along with the comment.
- // ```
- // #define MAKE_METHOD(name) \
- // /** Comment is here, inside the macro. */ \
- // void name() {}
- //
- // struct S {
- // MAKE_METHOD(f)
- // }
- // ```
- // 3. Macros that both declare a type and name a decl outside the macro.
- // ```
- // /// Comment is here, where we use the macro.
- // typedef NS_ENUM(NSInteger, Size) {
- // SizeWidth,
- // SizeHeight
- // };
- // ```
- // In this case NS_ENUM declares am enum type, and uses the same name for
- // the typedef declaration that appears outside the macro. The comment
- // here should be applied to both declarations inside and outside the
- // macro.
- //
- // We have found a Decl name that comes from inside a macro, but
- // Decl::getLocation() returns the place where the macro is being called.
- // If the declaration (and not just the name) resides inside the macro,
- // then we want to map Decl::getLocation() into the macro to where the
- // declaration and its attached comment (if any) were written.
- //
- // This mapping into the macro is done by mapping the location to its
- // spelling location, however even if the declaration is inside a macro,
- // the name's spelling can come from a macro argument (case 2 above). In
- // this case mapping the location to the spelling location finds the
- // argument's position (at `f` in MAKE_METHOD(`f`) above), which is not
- // where the declaration and its comment are located.
- //
- // To avoid this issue, we make use of Decl::getBeginLocation() instead.
- // While the declaration's position is where the name is written, the
- // comment is always attached to the begining of the declaration, not to
- // the name.
- //
- // In the first case, the begin location of the decl is outside the macro,
- // at the location of `typedef`. This is where the comment is found as
- // well. The begin location is not inside a macro, so it's spelling
- // location is the same.
- //
- // In the second case, the begin location of the decl is the call to the
- // macro, at `MAKE_METHOD`. However its spelling location is inside the
- // the macro at the location of `void`. This is where the comment is found
- // again.
- //
- // In the third case, there's no correct single behaviour. We want to use
- // the comment outside the macro for the definition that's inside the macro.
- // There is also a definition outside the macro, and we want the comment to
- // apply to both. The cases we care about here is NS_ENUM() and
- // NS_OPTIONS(). In general, if an enum is defined inside a macro, we should
- // try to find the comment there.
-
- // This is handling case 3 for NS_ENUM() and NS_OPTIONS(), which define
- // enum types inside the macro.
- if (isa<EnumDecl>(D)) {
- SourceLocation MacroCallLoc = SourceMgr.getExpansionLoc(DeclLoc);
- if (auto BufferRef =
- SourceMgr.getBufferOrNone(SourceMgr.getFileID(MacroCallLoc));
- BufferRef.has_value()) {
- llvm::StringRef buffer = BufferRef->getBuffer().substr(
- SourceMgr.getFileOffset(MacroCallLoc));
- if (buffer.starts_with("NS_ENUM(") ||
- buffer.starts_with("NS_OPTIONS(")) {
- // We want to use the comment on the call to NS_ENUM and NS_OPTIONS
- // macros for the types defined inside the macros, which is at the
- // expansion location.
- return MacroCallLoc;
- }
- }
- }
- return SourceMgr.getSpellingLoc(D->getBeginLoc());
+ if (!D->getLocation().isMacroID()) {
+ Locations.emplace_back(BaseLocation);
+ } else {
+ // When encountering definitions generated from a macro we need to try and
+ // find the comment at the location of the expansion but if there is no
+ // comment there we should retry to see if there is a comment inside the
+ // macro as well. To this end we return first Decl::getLocation() to first
+ // look at the expansion site, the second value is the spelling location of
+ // the beginning of the declaration defined inside the macro.
+ Locations.emplace_back(SourceMgr.getExpansionLoc(BaseLocation));
+ // We use Decl::getBeginLoc() and not just BaseLocation here to ensure that
+ // we don't refer to the macro argument location at the expansion site (this
+ // can happen if the name's spelling is provided via macro argument), and
+ // always to the declaration itself.
+ Locations.emplace_back(SourceMgr.getSpellingLoc(D->getBeginLoc()));
}
- return DeclLoc;
+ return Locations;
}
RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
@@ -357,30 +283,36 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
}
RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
- const SourceLocation DeclLoc = getDeclLocForCommentSearch(D, SourceMgr);
+ const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
- // If the declaration doesn't map directly to a location in a file, we
- // can't find the comment.
- if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
- return nullptr;
+ for (const auto DeclLoc : DeclLocs) {
+ // If the declaration doesn't map directly to a location in a file, we
+ // can't find the comment.
+ if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
+ continue;
- if (ExternalSource && !CommentsLoaded) {
- ExternalSource->ReadComments();
- CommentsLoaded = true;
- }
+ if (ExternalSource && !CommentsLoaded) {
+ ExternalSource->ReadComments();
+ CommentsLoaded = true;
+ }
- if (Comments.empty())
- return nullptr;
+ if (Comments.empty())
+ continue;
- const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
- if (!File.isValid()) {
- return nullptr;
+ const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
+ if (!File.isValid())
+ continue;
+
+ const auto CommentsInThisFile = Comments.getCommentsInFile(File);
+ if (!CommentsInThisFile || CommentsInThisFile->empty())
+ continue;
+
+ if (RawComment *Comment =
+ getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile))
+ return Comment;
}
- const auto CommentsInThisFile = Comments.getCommentsInFile(File);
- if (!CommentsInThisFile || CommentsInThisFile->empty())
- return nullptr;
- return getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile);
+ return nullptr;
}
void ASTContext::addComment(const RawComment &RC) {
@@ -592,19 +524,22 @@ void ASTContext::attachCommentsToJustParsedDecls(ArrayRef<Decl *> Decls,
D = &adjustDeclToTemplate(*D);
- const SourceLocation DeclLoc = getDeclLocForCommentSearch(D, SourceMgr);
+ const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
- if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
- continue;
+ for (const auto DeclLoc : DeclLocs) {
+ if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
+ continue;
- if (DeclRawComments.count(D) > 0)
- continue;
+ if (DeclRawComments.count(D) > 0)
+ continue;
- if (RawComment *const DocComment =
- getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile)) {
- cacheRawCommentForDecl(*D, *DocComment);
- comments::FullComment *FC = DocComment->parse(*this, PP, D);
- ParsedComments[D->getCanonicalDecl()] = FC;
+ if (RawComment *const DocComment = getRawCommentForDeclNoCacheImpl(
+ D, DeclLoc, *CommentsInThisFile)) {
+ cacheRawCommentForDecl(*D, *DocComment);
+ comments::FullComment *FC = DocComment->parse(*this, PP, D);
+ ParsedComments[D->getCanonicalDecl()] = FC;
+ break;
+ }
}
}
}
diff --git a/clang/test/Index/annotate-comments-objc.m b/clang/test/Index/annotate-comments-objc.m
index 6a48d9ae8f2cb3e..951e7e91418822a 100644
--- a/clang/test/Index/annotate-comments-objc.m
+++ b/clang/test/Index/annotate-comments-objc.m
@@ -56,7 +56,6 @@ void functionFromMacro(void) { \
typedef struct Struct_notdoxy Struct_notdoxy; \
}
-/// IS_DOXYGEN_NOT_ATTACHED
DECLARE_FUNCTIONS(WithSuffix)
/// typedef_isdoxy1 IS_DOXYGEN_SINGLE
@@ -68,7 +67,6 @@ void functionFromMacro(void) { \
/** namedEnumFromMacro IS_DOXYGEN_SINGLE */ \
enum name { B };
-/// IS_DOXYGEN_NOT_ATTACHED
DECLARE_ENUMS(namedEnumFromMacro)
#endif
@@ -133,8 +131,8 @@ void functionFromMacro(void) { \
// CHECK: annotate-comments-objc.m:41:22: EnumDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
// CHECK: annotate-comments-objc.m:41:22: TypedefDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
// CHECK: annotate-comments-objc.m:41:22: EnumDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
-// CHECK: annotate-comments-objc.m:60:1: FunctionDecl=functionFromMacro:{{.*}} BriefComment=[functionFromMacro IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:60:1: FunctionDecl=functionFromMacroWithSuffix:{{.*}} BriefComment=[functionFromMacroWithSuffix IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:63:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE
-// CHECK: annotate-comments-objc.m:72:1: EnumDecl=enumFromMacro:{{.*}} BriefComment=[enumFromMacro IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:72:15: EnumDecl=namedEnumFromMacro:{{.*}} BriefComment=[namedEnumFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:59:1: FunctionDecl=functionFromMacro:{{.*}} BriefComment=[functionFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:59:1: FunctionDecl=functionFromMacroWithSuffix:{{.*}} BriefComment=[functionFromMacroWithSuffix IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:62:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE
+// CHECK: annotate-comments-objc.m:70:1: EnumDecl=enumFromMacro:{{.*}} BriefComment=[enumFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:70:15: EnumDecl=namedEnumFromMacro:{{.*}} BriefComment=[namedEnumFromMacro IS_DOXYGEN_SINGLE]
diff --git a/clang/unittests/Tooling/SourceCodeTest.cpp b/clang/unittests/Tooling/SourceCodeTest.cpp
index 3d1dbceb63a7ffa..870d51c65907bf0 100644
--- a/clang/unittests/Tooling/SourceCodeTest.cpp
+++ b/clang/unittests/Tooling/SourceCodeTest.cpp
@@ -374,11 +374,10 @@ TEST(SourceCodeTest, getAssociatedRangeWithComments) {
// Does not include comments when only the decl or the comment come from a
// macro.
- // FIXME: Change code to allow this.
Visit(R"cpp(
#define DECL int x
- // Comment
- $r[[DECL;]])cpp");
+ $r[[// Comment
+ DECL;]])cpp");
Visit(R"cpp(
#define COMMENT /* Comment */
COMMENT
>From fa396058281da5d2c1c2221f247c99e51f208a45 Mon Sep 17 00:00:00 2001
From: Daniel Grumberg <dgrumberg at apple.com>
Date: Mon, 23 Oct 2023 14:37:05 +0100
Subject: [PATCH 2/2] Address PR feedback and add test checking logic for
comments outside macro for function
---
clang/lib/AST/ASTContext.cpp | 28 ++++++++++-------
clang/test/Index/annotate-comments-objc.m | 35 +++++++++++++---------
clang/unittests/Tooling/SourceCodeTest.cpp | 3 +-
3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 074f59bd5a50411..ec08a0d3ba7d84a 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -187,13 +187,20 @@ getDeclLocsForCommentSearch(const Decl *D, SourceManager &SourceMgr) {
if (!D->getLocation().isMacroID()) {
Locations.emplace_back(BaseLocation);
} else {
- // When encountering definitions generated from a macro we need to try and
- // find the comment at the location of the expansion but if there is no
- // comment there we should retry to see if there is a comment inside the
- // macro as well. To this end we return first Decl::getLocation() to first
- // look at the expansion site, the second value is the spelling location of
- // the beginning of the declaration defined inside the macro.
- Locations.emplace_back(SourceMgr.getExpansionLoc(BaseLocation));
+ const auto *DeclCtx = D->getDeclContext();
+
+ // When encountering definitions generated from a macro (that are not
+ // contained by another declaration in the macro) we need to try and find
+ // the comment at the location of the expansion but if there is no comment
+ // there we should retry to see if there is a comment inside the macro as
+ // well. To this end we return first BaseLocation to first look at the
+ // expansion site, the second value is the spelling location of the
+ // beginning of the declaration defined inside the macro.
+ if (!(DeclCtx &&
+ Decl::castFromDeclContext(DeclCtx)->getLocation().isMacroID())) {
+ Locations.emplace_back(SourceMgr.getExpansionLoc(BaseLocation));
+ }
+
// We use Decl::getBeginLoc() and not just BaseLocation here to ensure that
// we don't refer to the macro argument location at the expansion site (this
// can happen if the name's spelling is provided via macro argument), and
@@ -516,7 +523,6 @@ void ASTContext::attachCommentsToJustParsedDecls(ArrayRef<Decl *> Decls,
// declaration, but also comments that *follow* the declaration -- thanks to
// the lookahead in the lexer: we've consumed the semicolon and looked
// ahead through comments.
-
for (const Decl *D : Decls) {
assert(D);
if (D->isInvalidDecl())
@@ -524,15 +530,15 @@ void ASTContext::attachCommentsToJustParsedDecls(ArrayRef<Decl *> Decls,
D = &adjustDeclToTemplate(*D);
+ if (DeclRawComments.count(D) > 0)
+ continue;
+
const auto DeclLocs = getDeclLocsForCommentSearch(D, SourceMgr);
for (const auto DeclLoc : DeclLocs) {
if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
continue;
- if (DeclRawComments.count(D) > 0)
- continue;
-
if (RawComment *const DocComment = getRawCommentForDeclNoCacheImpl(
D, DeclLoc, *CommentsInThisFile)) {
cacheRawCommentForDecl(*D, *DocComment);
diff --git a/clang/test/Index/annotate-comments-objc.m b/clang/test/Index/annotate-comments-objc.m
index 951e7e91418822a..96bda58dfef20b9 100644
--- a/clang/test/Index/annotate-comments-objc.m
+++ b/clang/test/Index/annotate-comments-objc.m
@@ -46,17 +46,23 @@ - (void)method1_isdoxy4; /*!< method1_isdoxy4 IS_DOXYGEN_SINGLE */
// attach unrelated comments in the following cases where tag decls are
// embedded in declarators.
-#define DECLARE_FUNCTIONS(suffix) \
- /** functionFromMacro IS_DOXYGEN_SINGLE */ \
- void functionFromMacro(void) { \
- typedef struct Struct_notdoxy Struct_notdoxy; \
- } \
- /** functionFromMacroWithSuffix IS_DOXYGEN_SINGLE */ \
- void functionFromMacro##suffix(void) { \
- typedef struct Struct_notdoxy Struct_notdoxy; \
- }
-
-DECLARE_FUNCTIONS(WithSuffix)
+#define DECLARE_FUNCTIONS_COMMENTS_IN_MACRO(suffix) \
+ /** functionFromMacro IS_DOXYGEN_SINGLE */ \
+ void functionFromMacro(void) { \
+ typedef struct Struct_notdoxy Struct_notdoxy; \
+ } \
+ /** functionFromMacroWithSuffix IS_DOXYGEN_SINGLE */ \
+ void functionFromMacro##suffix(void) { \
+ typedef struct Struct_notdoxy Struct_notdoxy; \
+ }
+
+DECLARE_FUNCTIONS_COMMENTS_IN_MACRO(WithSuffix)
+
+#define DECLARE_FUNCTIONS \
+ void functionFromMacroWithCommentFromExpansionSite(void) { typedef struct Struct_notdoxy Struct_notdoxy; }
+
+/// functionFromMacroWithCommentFromExpansionSite IS_DOXYGEN_SINGLE
+DECLARE_FUNCTIONS
/// typedef_isdoxy1 IS_DOXYGEN_SINGLE
typedef struct Struct_notdoxy *typedef_isdoxy1;
@@ -133,6 +139,7 @@ void functionFromMacro(void) { \
// CHECK: annotate-comments-objc.m:41:22: EnumDecl=An_NS_ENUM_isdoxy1:{{.*}} An_NS_ENUM_isdoxy1 IS_DOXYGEN_SINGLE
// CHECK: annotate-comments-objc.m:59:1: FunctionDecl=functionFromMacro:{{.*}} BriefComment=[functionFromMacro IS_DOXYGEN_SINGLE]
// CHECK: annotate-comments-objc.m:59:1: FunctionDecl=functionFromMacroWithSuffix:{{.*}} BriefComment=[functionFromMacroWithSuffix IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:62:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE
-// CHECK: annotate-comments-objc.m:70:1: EnumDecl=enumFromMacro:{{.*}} BriefComment=[enumFromMacro IS_DOXYGEN_SINGLE]
-// CHECK: annotate-comments-objc.m:70:15: EnumDecl=namedEnumFromMacro:{{.*}} BriefComment=[namedEnumFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:65:1: FunctionDecl=functionFromMacroWithCommentFromExpansionSite:{{.*}} BriefComment=[functionFromMacroWithCommentFromExpansionSite IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:68:32: TypedefDecl=typedef_isdoxy1:{{.*}} typedef_isdoxy1 IS_DOXYGEN_SINGLE
+// CHECK: annotate-comments-objc.m:76:1: EnumDecl=enumFromMacro:{{.*}} BriefComment=[enumFromMacro IS_DOXYGEN_SINGLE]
+// CHECK: annotate-comments-objc.m:76:15: EnumDecl=namedEnumFromMacro:{{.*}} BriefComment=[namedEnumFromMacro IS_DOXYGEN_SINGLE]
diff --git a/clang/unittests/Tooling/SourceCodeTest.cpp b/clang/unittests/Tooling/SourceCodeTest.cpp
index 870d51c65907bf0..3641d2ee453f4a2 100644
--- a/clang/unittests/Tooling/SourceCodeTest.cpp
+++ b/clang/unittests/Tooling/SourceCodeTest.cpp
@@ -372,12 +372,11 @@ TEST(SourceCodeTest, getAssociatedRangeWithComments) {
#define DECL /* Comment */ int x
$r[[DECL;]])cpp");
- // Does not include comments when only the decl or the comment come from a
- // macro.
Visit(R"cpp(
#define DECL int x
$r[[// Comment
DECL;]])cpp");
+ // Does not include comments when only the comment come from a macro.
Visit(R"cpp(
#define COMMENT /* Comment */
COMMENT
More information about the cfe-commits
mailing list