Attaching comments to redeclarations: fix wrong assumptions

The reason for the recent fallout for "attaching comments to any redeclaration"
change are two false assumptions:
(1) a RawComment is attached to a single decl (not true for 'typedef struct X *Y'
    where we want the comment to be attached to both X and Y);
(2) the whole redeclaration chain has only a single comment (obviously false, the
    user can put a separate comment for each redeclaration).

To fix (1) I revert the part of the recent change where a 'Decl*' member was
introduced to RawComment.  Now ASTContext has a separate DenseMap for mapping
'Decl*' to 'FullComment*'.

To fix (2) I just removed the test with this assumption.  We might not parse
every comment in redecl chain if we already parsed at least one.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@161878 91177308-0d34-0410-b5e6-96231b3b80d8

6 files changed, 84 insertions, 51 deletions

diff --git a/include/clang/AST/ASTContext.h b/include/clang/AST/ASTContext.h
index 8fd7d6ef42..7b44d10f64 100644
--- a/include/clang/AST/ASTContext.h
+++ b/include/clang/AST/ASTContext.h
@@ -485,6 +485,10 @@ public:
   /// lazily.
   mutable llvm::DenseMap<const Decl *, RawCommentAndCacheFlags> RedeclComments;
 
+  /// \brief Mapping from declarations to parsed comments attached to any
+  /// redeclaration.
+  mutable llvm::DenseMap<const Decl *, comments::FullComment *> ParsedComments;
+
   /// \brief Return the documentation comment attached to a given declaration,
   /// without looking into cache.
   RawComment *getRawCommentForDeclNoCache(const Decl *D) const;
diff --git a/include/clang/AST/RawCommentList.h b/include/clang/AST/RawCommentList.h
index 4901d07c3c..630626b438 100644
--- a/include/clang/AST/RawCommentList.h
+++ b/include/clang/AST/RawCommentList.h
@@ -55,16 +55,11 @@ public:
 
   /// Is this comment attached to any declaration?
   bool isAttached() const LLVM_READONLY {
-    return !DeclOrParsedComment.isNull();
+    return IsAttached;
   }
 
-  /// Return the declaration that this comment is attached to.
-  const Decl *getDecl() const;
-
-  /// Set the declaration that this comment is attached to.
-  void setDecl(const Decl *D) {
-    assert(DeclOrParsedComment.isNull());
-    DeclOrParsedComment = D;
+  void setAttached() {
+    IsAttached = true;
   }
 
   /// Returns true if it is a comment that should be put after a member:
@@ -118,28 +113,23 @@ public:
     return extractBriefText(Context);
   }
 
-  /// Returns a \c FullComment AST node, parsing the comment if needed.
-  comments::FullComment *getParsed(const ASTContext &Context) const {
-    if (comments::FullComment *FC =
-            DeclOrParsedComment.dyn_cast<comments::FullComment *>())
-      return FC;
-
-    return parse(Context);
-  }
+  /// Parse the comment, assuming it is attached to decl \c D.
+  comments::FullComment *parse(const ASTContext &Context, const Decl *D) const;
 
 private:
   SourceRange Range;
 
   mutable StringRef RawText;
   mutable const char *BriefText;
-  mutable llvm::PointerUnion<const Decl *, comments::FullComment *>
-      DeclOrParsedComment;
 
   mutable bool RawTextValid : 1;   ///< True if RawText is valid
   mutable bool BriefTextValid : 1; ///< True if BriefText is valid
 
   unsigned Kind : 3;
 
+  /// True if comment is attached to a declaration in ASTContext.
+  bool IsAttached : 1;
+
   bool IsTrailingComment : 1;
   bool IsAlmostTrailingComment : 1;
 
@@ -152,7 +142,7 @@ private:
   RawComment(SourceRange SR, CommentKind K, bool IsTrailingComment,
              bool IsAlmostTrailingComment) :
     Range(SR), RawTextValid(false), BriefTextValid(false), Kind(K),
-    IsTrailingComment(IsTrailingComment),
+    IsAttached(false), IsTrailingComment(IsTrailingComment),
     IsAlmostTrailingComment(IsAlmostTrailingComment),
     BeginLineValid(false), EndLineValid(false)
   { }
@@ -161,8 +151,6 @@ private:
 
   const char *extractBriefText(const ASTContext &Context) const;
 
-  comments::FullComment *parse(const ASTContext &Context) const;
-
   friend class ASTReader;
 };
 
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
index ad48dffb4d..ae531e1b5b 100644
--- a/lib/AST/ASTContext.cpp
+++ b/lib/AST/ASTContext.cpp
@@ -66,6 +66,12 @@ RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
   if (D->isImplicit())
     return NULL;
 
+  // User can not attach documentation to implicit instantiations.
+  if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+    if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
+      return NULL;
+  }
+
   // TODO: handle comments for function parameters properly.
   if (isa<ParmVarDecl>(D))
     return NULL;
@@ -145,7 +151,6 @@ RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
         SourceMgr.getLineNumber(DeclLocDecomp.first, DeclLocDecomp.second)
           == SourceMgr.getLineNumber(CommentBeginDecomp.first,
                                      CommentBeginDecomp.second)) {
-      (*Comment)->setDecl(D);
       return *Comment;
     }
   }
@@ -185,13 +190,13 @@ RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
   if (Text.find_first_of(",;{}#@") != StringRef::npos)
     return NULL;
 
-   (*Comment)->setDecl(D);
   return *Comment;
 }
 
-const RawComment *ASTContext::getRawCommentForAnyRedecl(const Decl *D) const {
-  // If we have a 'templated' declaration for a template, adjust 'D' to
-  // refer to the actual template.
+namespace {
+/// If we have a 'templated' declaration for a template, adjust 'D' to
+/// refer to the actual template.
+const Decl *adjustDeclToTemplate(const Decl *D) {
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     if (const FunctionTemplateDecl *FTD = FD->getDescribedFunctionTemplate())
       D = FTD;
@@ -200,6 +205,12 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(const Decl *D) const {
       D = CTD;
   }
   // FIXME: Alias templates?
+  return D;
+}
+} // unnamed namespace
+
+const RawComment *ASTContext::getRawCommentForAnyRedecl(const Decl *D) const {
+  D = adjustDeclToTemplate(D);
 
   // Check whether we have cached a comment for this declaration already.
   {
@@ -259,11 +270,20 @@ const RawComment *ASTContext::getRawCommentForAnyRedecl(const Decl *D) const {
 }
 
 comments::FullComment *ASTContext::getCommentForDecl(const Decl *D) const {
+  D = adjustDeclToTemplate(D);
+  const Decl *Canonical = D->getCanonicalDecl();
+  llvm::DenseMap<const Decl *, comments::FullComment *>::iterator Pos =
+      ParsedComments.find(Canonical);
+  if (Pos != ParsedComments.end())
+    return Pos->second;
+
   const RawComment *RC = getRawCommentForAnyRedecl(D);
   if (!RC)
     return NULL;
 
-  return RC->getParsed(*this);
+  comments::FullComment *FC = RC->parse(*this, D);
+  ParsedComments[Canonical] = FC;
+  return FC;
 }
 
 void 
diff --git a/lib/AST/RawCommentList.cpp b/lib/AST/RawCommentList.cpp
index c704cabe69..a5a3287057 100644
--- a/lib/AST/RawCommentList.cpp
+++ b/lib/AST/RawCommentList.cpp
@@ -65,7 +65,7 @@ bool mergedCommentIsTrailingComment(StringRef Comment) {
 RawComment::RawComment(const SourceManager &SourceMgr, SourceRange SR,
                        bool Merged) :
     Range(SR), RawTextValid(false), BriefTextValid(false),
-    IsAlmostTrailingComment(false),
+    IsAttached(false), IsAlmostTrailingComment(false),
     BeginLineValid(false), EndLineValid(false) {
   // Extract raw comment text, if possible.
   if (SR.getBegin() == SR.getEnd() || getRawText(SourceMgr).empty()) {
@@ -87,16 +87,6 @@ RawComment::RawComment(const SourceManager &SourceMgr, SourceRange SR,
   }
 }
 
-const Decl *RawComment::getDecl() const {
-  if (DeclOrParsedComment.isNull())
-    return NULL;
-
-  if (const Decl *D = DeclOrParsedComment.dyn_cast<const Decl *>())
-    return D;
-
-  return DeclOrParsedComment.get<comments::FullComment *>()->getDecl();
-}
-
 unsigned RawComment::getBeginLine(const SourceManager &SM) const {
   if (BeginLineValid)
     return BeginLine;
@@ -169,7 +159,8 @@ const char *RawComment::extractBriefText(const ASTContext &Context) const {
   return BriefTextPtr;
 }
 
-comments::FullComment *RawComment::parse(const ASTContext &Context) const {
+comments::FullComment *RawComment::parse(const ASTContext &Context,
+                                         const Decl *D) const {
   // Make sure that RawText is valid.
   getRawText(Context.getSourceManager());
 
@@ -179,13 +170,11 @@ comments::FullComment *RawComment::parse(const ASTContext &Context) const {
                     RawText.begin(), RawText.end());
   comments::Sema S(Context.getAllocator(), Context.getSourceManager(),
                    Context.getDiagnostics(), Traits);
-  S.setDecl(getDecl());
+  S.setDecl(D);
   comments::Parser P(L, S, Context.getAllocator(), Context.getSourceManager(),
                      Context.getDiagnostics(), Traits);
 
-  comments::FullComment *FC = P.parseFullComment();
-  DeclOrParsedComment = FC;
-  return FC;
+  return P.parseFullComment();
 }
 
 namespace {
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 3aae99ab74..b014b4cd9f 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -7623,7 +7623,9 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D) {
         << FD->getName() << "dllimport";
     }
   }
-  ActOnDocumentableDecl(FD);
+  // We want to attach documentation to original Decl (which might be
+  // a function template).
+  ActOnDocumentableDecl(D);
   return FD;
 }
 
diff --git a/test/Sema/warn-documentation.cpp b/test/Sema/warn-documentation.cpp
index 16ba4297cd..a361d57baa 100644
--- a/test/Sema/warn-documentation.cpp
+++ b/test/Sema/warn-documentation.cpp
@@ -624,13 +624,9 @@ class test_attach37 {
   /// \brief\author Aaa
   /// \tparam T Aaa
   void test_attach38(int aaa, int bbb);
-};
 
-// expected-warning@+1 {{empty paragraph passed to '\brief' command}}
-/// \brief\author Aaa
-/// \tparam T Aaa
-template<typename T>
-void test_attach37<T>::test_attach38(int aaa, int bbb) {}
+  void test_attach39(int aaa, int bbb);
+};
 
 // expected-warning@+2 {{empty paragraph passed to '\brief' command}}
 // expected-warning@+2 {{template parameter 'T' not found in the template declaration}}
@@ -639,6 +635,13 @@ void test_attach37<T>::test_attach38(int aaa, int bbb) {}
 template<>
 void test_attach37<int>::test_attach38(int aaa, int bbb) {}
 
+// expected-warning@+1 {{empty paragraph passed to '\brief' command}}
+/// \brief\author Aaa
+/// \tparam T Aaa
+template<typename T>
+void test_attach37<T>::test_attach39(int aaa, int bbb) {}
+
+
 
 // PR13411, reduced.  We used to crash on this.
 /**
@@ -652,7 +655,7 @@ void test_nocrash1(int);
 /// \param\brief
 void test_nocrash2(int);
 
-// PR13593
+// PR13593, example 1 and 2
 
 /**
 * Bla.
@@ -668,3 +671,30 @@ template <typename>
 void test_nocrash3()
 {
 }
+
+// PR13593, example 3
+
+/**
+ * aaa
+ */
+template <typename T>
+inline T test_nocrash5(T a1)
+{
+    return a1;
+}
+
+///
+//,
+
+inline void test_nocrash6()
+{
+    test_nocrash5(1);
+}
+
+// We used to crash on this.
+
+/*!
+  Blah.
+*/
+typedef const struct test_nocrash7 * test_nocrash8;
+