[clang] A more robust way to attach comments

[zixuw]

Summary

The current implementation in getRawCommentForDeclNoCacheImpl to check
if a leading comment is attached to a given Decl *D is to see if there
is any of ";{}#@" between the comment and the source location of D,
which indicates that there's other declarations or preprocessor
directives that the comment is meant for. However, this does not work
for enum constants, which are separated by commas.
Comma was originally included in the filter characters list, but removed
in b534d3a0ef69 because macros, attributes and other decorators might
also introduce commas before a declaration. And there's no good way to
reliably and efficiently check if all the commas are from other
declarations.
This patch added a forward pointer in DeclBase pointing to the
previous declaration inside the lexical DeclContext, similar to the
existing NextInContextAndBits. This constructs a doubly linked list so
we can easily query the lexically preceding declaration of the current
decl D, to check if any decl falls between the comment and D.

[zixuw]

Well, apparently there are still some corner-case bugs in the logic. ExtractAPI and a few other tests failed. Will look into it tomorrow.

And if folks have concerns or opinions of the performance/space cost of adding the extra pointer to DeclBase, I can try to keep the original logic and just special-case enum constants to find the previous enum constant decl in the parent enum. Not sure if there are existing ways to get preceding enum constants efficiently, otherwise it'd be an O(n^2) operation for an enum.

[akyrtzi]

Since the issue is specific to enumerators I would recommend against increasing the size of DeclBase, which would increase the size of every single Decl in the AST.

There are multiple ways to approach this:

• Consider whether bringing back the comma check just for enumerators would work.
• Check the enumerators from the parent EnumDecl. The number of enumerators inside an EnumDecl is usually relatively small, you can do measurements to see how the performance looks with actual code.
• If there is noticable performance regression:
  • Either add "previous enumerator" pointer in EnumConstantDecl (increases size of every EnumConstantDecl in the AST)
  • Or add "out-of-AST-decl" tracking via adding some DenseMap in ASTContext that is only populated when comments for enumerators of an EnumDecl are requested. There is much precedent for this, for example see these examples:

	  /// A cache mapping from CXXRecordDecls to key functions.
	  llvm::DenseMap<const CXXRecordDecl*, LazyDeclPtr> KeyFunctions;

	  /// Mapping from ObjCContainers to their ObjCImplementations.
	  llvm::DenseMap<ObjCContainerDecl*, ObjCImplDecl*> ObjCImpls;

	  /// Mapping from ObjCMethod to its duplicate declaration in the same
	  /// interface.
	  llvm::DenseMap<const ObjCMethodDecl*,const ObjCMethodDecl*> ObjCMethodRedecls;

	  /// Mapping from __block VarDecls to BlockVarCopyInit.
	  llvm::DenseMap<const VarDecl *, BlockVarCopyInit> BlockVarCopyInits;

The benefit of a separate map is that it only takes additional memory when a particular functionality is requested.

clang/include/clang/AST/DeclBase.h

 protected:
   /// The next declaration within the same lexical
   /// DeclContext. These pointers form the linked list that is
   /// traversed via DeclContext's decls_begin()/decls_end().
   ///
   /// The extra two bits are used for the ModuleOwnershipKind.
   llvm::PointerIntPair<Decl *, 2, ModuleOwnershipKind> NextInContextAndBits;
 
+  /// The previous declaration within the same lexical DeclContext.
+  Decl *PreviousInContext;
+
 private:
   friend class DeclContext;
 
   struct MultipleDC {
     DeclContext *SemanticDC;
     DeclContext *LexicalDC;
   };
 
public:
   Decl(const Decl&) = delete;
   Decl(Decl &&) = delete;
   Decl &operator=(const Decl&) = delete;
   Decl &operator=(Decl&&) = delete;
 
 protected:
   Decl(Kind DK, DeclContext *DC, SourceLocation L)
       : NextInContextAndBits(nullptr, getModuleOwnershipKindForChildOf(DC)),
-        DeclCtx(DC), Loc(L), DeclKind(DK), InvalidDecl(false), HasAttrs(false),
-        Implicit(false), Used(false), Referenced(false),
-        TopLevelDeclInObjCContainer(false), Access(AS_none), FromASTFile(0),
-        IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
+        PreviousInContext(nullptr), DeclCtx(DC), Loc(L), DeclKind(DK),
+        InvalidDecl(false), HasAttrs(false), Implicit(false), Used(false),
+        Referenced(false), TopLevelDeclInObjCContainer(false), Access(AS_none),
+        FromASTFile(0), IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
         CacheValidAndLinkage(0) {
-    if (StatisticsEnabled) add(DK);
+    if (StatisticsEnabled)
+      add(DK);
   }
 
   Decl(Kind DK, EmptyShell Empty)
-      : DeclKind(DK), InvalidDecl(false), HasAttrs(false), Implicit(false),
-        Used(false), Referenced(false), TopLevelDeclInObjCContainer(false),
-        Access(AS_none), FromASTFile(0),
+      : PreviousInContext(nullptr), DeclKind(DK), InvalidDecl(false),
+        HasAttrs(false), Implicit(false), Used(false), Referenced(false),
+        TopLevelDeclInObjCContainer(false), Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
         CacheValidAndLinkage(0) {
-    if (StatisticsEnabled) add(DK);
+    if (StatisticsEnabled)
+      add(DK);
   }
 
   virtual ~Decl();
 
   /// Update a potentially out-of-date declaration.
   void updateOutOfDate(IdentifierInfo &II) const;
 
   Linkage getCachedLinkage() const {
public:
   void setLocation(SourceLocation L) { Loc = L; }
 
   Kind getKind() const { return static_cast<Kind>(DeclKind); }
   const char *getDeclKindName() const;
 
   Decl *getNextDeclInContext() { return NextInContextAndBits.getPointer(); }
   const Decl *getNextDeclInContext() const {return NextInContextAndBits.getPointer();}
 
+  Decl *getPreviousDeclInContext() { return PreviousInContext; }
+  const Decl *getPreviousDeclInContext() const { return PreviousInContext; }
+
   DeclContext *getDeclContext() {
     if (isInSemaDC())
       return getSemanticDC();
     return getMultipleDC()->SemanticDC;
   }
   const DeclContext *getDeclContext() const {
     return const_cast<Decl*>(this)->getDeclContext();
   }

clang/lib/AST/ASTContext.cpp

RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
   RawComment *CommentBeforeDecl = OffsetCommentBeforeDecl->second;
 
   // Check that we actually have a non-member Doxygen comment.
   if (!(CommentBeforeDecl->isDocumentation() ||
         LangOpts.CommentOpts.ParseAllComments) ||
       CommentBeforeDecl->isTrailingComment())
     return nullptr;
 
+  const SourceLocation &CommentEndLoc = CommentBeforeDecl->getEndLoc();
+  if (CommentEndLoc.isInvalid())
+    return nullptr;
+
+  // Check if there's any other declaration in between the end of the comment
+  // and the current declaration.
+  if (const Decl *PreviousInContext = D->getPreviousDeclInContext()) {
+    // Use the previous decl's declaration location if the previous declaration
+    // is the lexical declaration context of the current declaration, and the
+    // end location otherwise. For example:
+    //
+    // ```
+    //   /// Doc for struct s
+    //   struct s {          // <- declaration location for `s`
+    //
+    //     int i;            // Both the PreviousDeclInContext and the
+    //                       // LexicalDeclContext of `i` is `s`
+    //
+    //     /// Doc for func
+    //     void func();
+    //   };                  // <- end location for `s`
+    //
+    //   bool b;             // The PreviousDeclInContext of `b` is `s`
+    // ```
+    //
+    // In this case, `Doc for struct s` shouldn't be attached to `i`; and
+    // `Doc for func` shouldn't be attached to `b`.
+    const DeclContext *DC = dyn_cast<DeclContext>(PreviousInContext);
+    const SourceLocation &PreviousDeclLoc =
+        DC && D->getLexicalDeclContext() == DC
+            ? PreviousInContext->getLocation()
+            : PreviousInContext->getEndLoc();
+    if (PreviousDeclLoc.isValid() && PreviousDeclLoc.isFileID() &&
+        SourceMgr.isPointWithin(PreviousDeclLoc, CommentEndLoc,
+                                RepresentativeLocForDecl))
+      return nullptr;
+  }
+
   // Decompose the end of the comment.
   const unsigned CommentEndOffset =
       Comments.getCommentEndOffset(CommentBeforeDecl);
 
   // Get the corresponding buffer.
   bool Invalid = false;
   const char *Buffer = SourceMgr.getBufferData(DeclLocDecomp.first,
                                                &Invalid).data();
   if (Invalid)
     return nullptr;
 
   // Extract text between the comment and declaration.
   StringRef Text(Buffer + CommentEndOffset,
                  DeclLocDecomp.second - CommentEndOffset);
 
-  // There should be no other declarations or preprocessor directives between
-  // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  // There should be no preprocessor directives between comment and declaration.
+  if (Text.find_first_of("#@") != StringRef::npos)
     return nullptr;
 
   return CommentBeforeDecl;
 }
 
 RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
   const SourceLocation DeclLoc = getDeclLocForCommentSearch(D, SourceMgr);
 

clang/lib/AST/DeclBase.cpp

for (auto *D : Decls) {
     if (FieldsAlreadyLoaded && isa<FieldDecl>(D))
       continue;
 
     if (PrevDecl)
       PrevDecl->NextInContextAndBits.setPointer(D);
     else
       FirstNewDecl = D;
 
+    D->PreviousInContext = PrevDecl;
     PrevDecl = D;
   }
 
   return std::make_pair(FirstNewDecl, PrevDecl);
 }
 
 /// We have just acquired external visible storage, and we already have
 /// built a lookup map. For every name in the map, pull in the new names from
if (const auto *RD = dyn_cast<RecordDecl>(this))
     FieldsAlreadyLoaded = RD->hasLoadedFieldsFromExternalStorage();
 
   // Splice the newly-read declarations into the beginning of the list
   // of declarations.
   Decl *ExternalFirst, *ExternalLast;
   std::tie(ExternalFirst, ExternalLast) =
       BuildDeclChain(Decls, FieldsAlreadyLoaded);
   ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  if (FirstDecl)
+    FirstDecl->PreviousInContext = ExternalLast;
   FirstDecl = ExternalFirst;
   if (!LastDecl)
     LastDecl = ExternalLast;
   return true;
 }
 
 DeclContext::lookup_result
 ExternalASTSource::SetNoExternalVisibleDeclsForName(const DeclContext *DC,

 }
 
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
          "decl being removed from non-lexical context");
   assert((D->NextInContextAndBits.getPointer() || D == LastDecl) &&
          "decl is not in decls list");
 
-  // Remove D from the decl chain.  This is O(n) but hopefully rare.
+  // Remove D from the decl chain.
   if (D == FirstDecl) {
     if (D == LastDecl)
       FirstDecl = LastDecl = nullptr;
     else
       FirstDecl = D->NextInContextAndBits.getPointer();
-  } else {
-    for (Decl *I = FirstDecl; true; I = I->NextInContextAndBits.getPointer()) {
-      assert(I && "decl not found in linked list");
-      if (I->NextInContextAndBits.getPointer() == D) {
-        I->NextInContextAndBits.setPointer(D->NextInContextAndBits.getPointer());
-        if (D == LastDecl) LastDecl = I;
-        break;
-      }
-    }
+
+  if (auto *Next = D->NextInContextAndBits.getPointer())
+    Next->PreviousInContext = D->PreviousInContext;
+
+  if (auto *Prev = D->PreviousInContext) {
+    Prev->NextInContextAndBits.setPointer(D->NextInContextAndBits.getPointer());
+    if (D == LastDecl)
+      LastDecl = Prev;
   }
 
   // Mark that D is no longer in the decl chain.
   D->NextInContextAndBits.setPointer(nullptr);
+  D->PreviousInContext = nullptr;
 
   // Remove D from the lookup table if necessary.
   if (isa<NamedDecl>(D)) {
     auto *ND = cast<NamedDecl>(D);
 
     // Do not try to remove the declaration if that is invisible to qualified
     // lookup.  E.g. template specializations are skipped.
     if (shouldBeHidden(ND))

 void DeclContext::addHiddenDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
          "Decl inserted into wrong lexical context");
   assert(!D->getNextDeclInContext() && D != LastDecl &&
          "Decl already inserted into a DeclContext");
 
   if (FirstDecl) {
     LastDecl->NextInContextAndBits.setPointer(D);
+    D->PreviousInContext = LastDecl;
     LastDecl = D;
   } else {
     FirstDecl = LastDecl = D;
   }
 
   // Notify a C++ record declaration that we've added a member, so it can
   // update its class-specific state.
   if (auto *Record = dyn_cast<CXXRecordDecl>(this))

clang/test/Index/annotate-comments-enum-constant.c

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: c-index-test -test-load-source all -comments-xml-schema=%S/../../bindings/xml/comment-xml-schema.rng %s > %t/out
+// RUN: FileCheck %s --dump-input always < %t/out
+
+enum {
+  /// Documentation for Foo
+  Foo,
+  Bar, // No documentation for Bar
+  /// Documentation for Baz
+  Baz,
+};
+// CHECK: EnumConstantDecl=Foo:[[@LINE-5]]:3 (Definition) {{.*}} BriefComment=[Documentation for Foo] FullCommentAsHTML=[<p class="para-brief"> Documentation for Foo</p>] FullCommentAsXML=[<Variable file="{{[^"]+}}annotate-comments-enum-constant.c" line="[[@LINE-5]]" column="3"><Name>Foo</Name><USR>c:@Ea@Foo@Foo</USR><Declaration>Foo</Declaration><Abstract><Para> Documentation for Foo</Para></Abstract></Variable>]
+// CHECK: EnumConstantDecl=Bar:[[@LINE-5]]:3 (Definition)
+// CHECK-NOT: BriefComment=[Documentation for Foo]
+// CHECK: EnumConstantDecl=Baz:[[@LINE-5]]:3 (Definition) {{.*}} BriefComment=[Documentation for Baz] FullCommentAsHTML=[<p class="para-brief"> Documentation for Baz</p>] FullCommentAsXML=[<Variable file="{{[^"]+}}annotate-comments-enum-constant.c" line="[[@LINE-5]]" column="3"><Name>Baz</Name><USR>c:@Ea@Foo@Baz</USR><Declaration>Baz</Declaration><Abstract><Para> Documentation for Baz</Para></Abstract></Variable>]

clang/test/Index/annotate-comments.cpp

 
 #define MYMAC(x,y)
 /**
  * Aaa. IS_DOXYGEN_START IS_DOXYGEN_END
  */
 MYMAC(0,0)
 void isdoxy54(int);
 
+/// IS_DOXYGEN_SINGLE
+struct isdoxy55 {
+  int notdoxy48;
+  /// IS_DOXYGEN_SINGLE
+  int isdoxy56;
+};
+bool notdoxy49;
+
 #endif
 
 // RUN: rm -rf %t
 // RUN: mkdir %t
 
 // Check that we serialize comment source locations properly.
 // RUN: %clang_cc1 -x c++ -std=c++11 -emit-pch -o %t/out.pch %s
 // RUN: %clang_cc1 -x c++ -std=c++11 -include-pch %t/out.pch -fsyntax-only %s

 // CHECK: annotate-comments.cpp:205:6: FunctionDecl=isdoxy46:{{.*}} BriefComment=[Ddd eee. Fff.]
 // 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]
 // CHECK: annotate-comments.cpp:231:6: FunctionDecl=isdoxy51:{{.*}} BriefComment=[Aaa. IS_DOXYGEN_START]
 // CHECK: annotate-comments.cpp:241:6: FunctionDecl=isdoxy52:{{.*}} BriefComment=[Aaa. IS_DOXYGEN_START Bbb.]
 // CHECK: annotate-comments.cpp:248:6: FunctionDecl=isdoxy53:{{.*}} BriefComment=[Aaa. IS_DOXYGEN_START IS_DOXYGEN_END]
 // CHECK: annotate-comments.cpp:255:6: FunctionDecl=isdoxy54:{{.*}} BriefComment=[Aaa. IS_DOXYGEN_START IS_DOXYGEN_END]
+
+// CHECK: annotate-comments.cpp:258:8: StructDecl=isdoxy55:{{.*}} IS_DOXYGEN_SINGLE
+// CHECK: annotate-comments.cpp:261:7: FieldDecl=isdoxy56:{{.*}} IS_DOXYGEN_SINGLE