From b261088a618d17ba71d8982093c8767da0e0e67a Mon Sep 17 00:00:00 2001 From: Dmitri Gribenko Date: Tue, 14 Aug 2012 17:17:18 +0000 Subject: [PATCH] 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. llvm-svn: 161878 --- clang/include/clang/AST/ASTContext.h | 4 +++ clang/include/clang/AST/RawCommentList.h | 30 +++++----------- clang/lib/AST/ASTContext.cpp | 32 +++++++++++++---- clang/lib/AST/RawCommentList.cpp | 21 +++-------- clang/lib/Sema/SemaDecl.cpp | 4 ++- clang/test/Sema/warn-documentation.cpp | 44 ++++++++++++++++++++---- 6 files changed, 84 insertions(+), 51 deletions(-) diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index 8fd7d6ef4233..7b44d10f6489 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -485,6 +485,10 @@ public: /// lazily. mutable llvm::DenseMap RedeclComments; + /// \brief Mapping from declarations to parsed comments attached to any + /// redeclaration. + mutable llvm::DenseMap ParsedComments; + /// \brief Return the documentation comment attached to a given declaration, /// without looking into cache. RawComment *getRawCommentForDeclNoCache(const Decl *D) const; diff --git a/clang/include/clang/AST/RawCommentList.h b/clang/include/clang/AST/RawCommentList.h index 4901d07c3cdc..630626b438ee 100644 --- a/clang/include/clang/AST/RawCommentList.h +++ b/clang/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()) - 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 - 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/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index ad48dffb4d4f..ae531e1b5b67 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/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(D)) { + if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation) + return NULL; + } + // TODO: handle comments for function parameters properly. if (isa(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(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::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/clang/lib/AST/RawCommentList.cpp b/clang/lib/AST/RawCommentList.cpp index c704cabe69f7..a5a32870577a 100644 --- a/clang/lib/AST/RawCommentList.cpp +++ b/clang/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()) - return D; - - return DeclOrParsedComment.get()->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/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 3aae99ab74e2..b014b4cd9fcd 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/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/clang/test/Sema/warn-documentation.cpp b/clang/test/Sema/warn-documentation.cpp index 16ba4297cdc4..a361d57baa79 100644 --- a/clang/test/Sema/warn-documentation.cpp +++ b/clang/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 -void test_attach37::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::test_attach38(int aaa, int bbb) {} template<> void test_attach37::test_attach38(int aaa, int bbb) {} +// expected-warning@+1 {{empty paragraph passed to '\brief' command}} +/// \brief\author Aaa +/// \tparam T Aaa +template +void test_attach37::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 void test_nocrash3() { } + +// PR13593, example 3 + +/** + * aaa + */ +template +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; +