This is mostly done in r165771. Please see bellow for couple of comments. - fariborz
On Oct 11, 2012, at 11:44 AM, Douglas Gregor <[email protected]> wrote: > > On Oct 10, 2012, at 11:34 AM, Fariborz Jahanian <[email protected]> wrote: > >> Author: fjahanian >> Date: Wed Oct 10 13:34:52 2012 >> New Revision: 165643 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=165643&view=rev >> Log: >> [Doc parsing] This patch searches overridden objc/c++ >> methods looking for documentation on a particular base >> class inherited by any method that overrides the base class. >> In case of redeclaration, as when objc method is defined >> in the implementation, it also looks up for documentation >> in class/class extension being redeclared. >> >> Added: >> cfe/trunk/test/Index/overriding-method-comments.mm >> Modified: >> cfe/trunk/include/clang/AST/Comment.h >> cfe/trunk/include/clang/AST/DeclObjC.h >> cfe/trunk/lib/AST/ASTContext.cpp >> cfe/trunk/lib/AST/Comment.cpp >> cfe/trunk/lib/AST/CommentDumper.cpp >> cfe/trunk/lib/AST/CommentSema.cpp >> cfe/trunk/lib/AST/DeclObjC.cpp >> cfe/trunk/tools/libclang/CXComment.cpp >> cfe/trunk/unittests/AST/CommentParser.cpp >> >> Modified: cfe/trunk/include/clang/AST/Comment.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Comment.h?rev=165643&r1=165642&r2=165643&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/AST/Comment.h (original) >> +++ cfe/trunk/include/clang/AST/Comment.h Wed Oct 10 13:34:52 2012 >> @@ -17,6 +17,7 @@ >> #include "clang/Basic/SourceLocation.h" >> #include "clang/AST/Type.h" >> #include "clang/AST/CommentCommandTraits.h" >> +#include "clang/AST/DeclObjC.h" >> #include "llvm/ADT/ArrayRef.h" >> #include "llvm/ADT/StringRef.h" >> >> @@ -727,7 +728,17 @@ >> return getNumArgs() > 0; >> } >> >> - StringRef getParamName() const { >> + StringRef getParamName(const Decl *OverridingDecl) const { > > How about passing the FullComment down, since that's what the user actually > has to consider? Also, it will help with my next comment… > >> + if (OverridingDecl && isParamIndexValid()) { >> + if (const ObjCMethodDecl *OMD = >> dyn_cast<ObjCMethodDecl>(OverridingDecl)) { >> + const ParmVarDecl *ParamDecl = OMD->param_begin()[getParamIndex()]; >> + return ParamDecl->getName(); >> + } >> + else if (const FunctionDecl *FD = >> dyn_cast<FunctionDecl>(OverridingDecl)) { >> + const ParmVarDecl *ParamDecl = FD->param_begin()[getParamIndex()]; >> + return ParamDecl->getName(); >> + } >> + } > > The parameters are actually already stored in the FullComment's DeclInfo. I > suggest going there instead, so we don't have to match the various forms of > OverridingDecl. This would also let you fix the redeclaration case fairly > easily, e.g., > > /// \brief Adds two integers > /// \param lhs The left-hand side > /// \param rhs The right-hand side > /// \returns the sum of \p lhs and \p rhs > int add(int lhs, int rhs); > > int add(int x, int y); // comment block for this will refer to > parameters 'lhs' and 'rhs', but should refer to x and y > > and even perhaps for typedefs (!) > > /// \brief Adds two integers > /// \param lhs The left-hand side > /// \param rhs The right-hand side > /// \returns the sum of \p lhs and \p rhs > typedef int adder(int lhs, int rhs); > typedef int adder(int x, int y); > > >> @@ -936,22 +947,22 @@ >> /// Information about the declaration, useful to clients of FullComment. >> struct DeclInfo { >> /// Declaration the comment is attached to. Should not be NULL. >> - const Decl *ThisDecl; >> - >> - /// Parameters that can be referenced by \\param if \c ThisDecl is >> something >> + const Decl *CommentDecl; >> + > > These are great, thanks! > >> Modified: cfe/trunk/lib/AST/ASTContext.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=165643&r1=165642&r2=165643&view=diff >> ============================================================================== >> --- cfe/trunk/lib/AST/ASTContext.cpp (original) >> +++ cfe/trunk/lib/AST/ASTContext.cpp Wed Oct 10 13:34:52 2012 >> @@ -24,6 +24,7 @@ >> #include "clang/AST/ASTMutationListener.h" >> #include "clang/AST/RecordLayout.h" >> #include "clang/AST/Mangle.h" >> +#include "clang/AST/Comment.h" >> #include "clang/Basic/Builtins.h" >> #include "clang/Basic/SourceManager.h" >> #include "clang/Basic/TargetInfo.h" >> @@ -355,21 +356,72 @@ >> return RC; >> } >> >> +static void addRedeclaredMethods(const ObjCMethodDecl *ObjCMethod, >> + SmallVectorImpl<const NamedDecl *> &Redeclared) { >> + const DeclContext *DC = ObjCMethod->getDeclContext(); >> + if (const ObjCImplDecl *IMD = dyn_cast<ObjCImplDecl>(DC)) { >> + const ObjCInterfaceDecl *ID = IMD->getClassInterface(); >> + if (!ID) >> + return; >> + // Add redeclared method here. >> + for (const ObjCCategoryDecl *ClsExtDecl = ID->getFirstClassExtension(); >> + ClsExtDecl; ClsExtDecl = ClsExtDecl->getNextClassExtension()) { >> + if (ObjCMethodDecl *RedeclaredMethod = >> + ClsExtDecl->getMethod(ObjCMethod->getSelector(), >> + ObjCMethod->isInstanceMethod())) >> + Redeclared.push_back(RedeclaredMethod); >> + } >> + } >> +} >> + > > Isn't there some pre-existing function you can re-use to find the redeclared > methods? It seems that Sema must do this already. AFAIK, there is no routine which does this. This routine searches for matching methods in class's extensions. Because they are redeclared in the implementation. All routines in Sema are of a more general nature. > >> comments::FullComment *ASTContext::getCommentForDecl( >> const Decl *D, >> const Preprocessor *PP) const { >> D = adjustDeclToTemplate(D); >> + >> const Decl *Canonical = D->getCanonicalDecl(); >> llvm::DenseMap<const Decl *, comments::FullComment *>::iterator Pos = >> ParsedComments.find(Canonical); >> - if (Pos != ParsedComments.end()) >> + >> + if (Pos != ParsedComments.end()) { >> + if (Canonical != D && >> + (isa<ObjCMethodDecl>(D) || isa<FunctionDecl>(D))) { > > Why do we need the isa<> checks here? This seems like a fairly general > approach to having the CommentDecl and the ThisDecl be different. Heck, we > might want it for, e.g., > > /// \brief A wonderful comment > @interface A > @end > > @class A; // should be able to get docs here! > @implementation A // and here! > @end > > >> + // case of method being redeclaration of the canonical, not >> + // overriding it; i.e. method in implementation, canonical in >> + // interface. Or, out-of-line cxx-method definition. >> + comments::FullComment *FC = Pos->second; >> + comments::FullComment *CFC = >> + new (*this) comments::FullComment(FC->getBlocks(), >> + FC->getThisDeclInfo(), >> + const_cast<Decl *>(D)); > > I think you want to create a new DeclInfo for the cloned FullComment, based > on the actual declaration (ThisDecl) rather than the comment declaration > (CommentDecl). Obviously, it's CommentDecl will be the same as the original > FullComment's CommentDecl. > >> + return CFC; >> + } >> return Pos->second; >> - >> + } >> + >> const Decl *OriginalDecl; >> + >> const RawComment *RC = getRawCommentForAnyRedecl(D, &OriginalDecl); >> - if (!RC) >> + if (!RC) { >> + if (isa<ObjCMethodDecl>(D) || isa<FunctionDecl>(D)) { >> + SmallVector<const NamedDecl*, 8> overridden; >> + if (const ObjCMethodDecl *OMD = dyn_cast<ObjCMethodDecl>(D)) >> + addRedeclaredMethods(OMD, overridden); >> + const_cast<ASTContext >> *>(this)->getOverriddenMethods(dyn_cast<NamedDecl>(D), >> + overridden); >> + for (unsigned i = 0, e = overridden.size(); i < e; i++) { >> + if (comments::FullComment *FC = getCommentForDecl(overridden[i], >> PP)) { >> + comments::FullComment *CFC = >> + new (*this) comments::FullComment(FC->getBlocks(), >> + FC->getThisDeclInfo(), >> + const_cast<Decl *>(D)); >> + return CFC; >> + } >> + } >> + } >> return NULL; > > This comment-cloning code is duplicated; please pull it out into a helper > routine. I suspect, based on my @class/@interface/@implementation example > above, that we'll want to have a few other cases here for "search for a > comment on a related declaration". > >> >> Modified: cfe/trunk/lib/AST/CommentDumper.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/CommentDumper.cpp?rev=165643&r1=165642&r2=165643&view=diff >> ============================================================================== >> --- cfe/trunk/lib/AST/CommentDumper.cpp (original) >> +++ cfe/trunk/lib/AST/CommentDumper.cpp Wed Oct 10 13:34:52 2012 >> @@ -183,7 +183,7 @@ >> OS << " implicitly"; >> >> if (C->hasParamName()) >> - OS << " Param=\"" << C->getParamName() << "\""; >> + OS << " Param=\"" << C->getParamName(0) << "\""; > > We should be able to pass down the FullComment here, right? FullComment is not available here. I will talk to you. - Fariborz > >> Added: cfe/trunk/test/Index/overriding-method-comments.mm >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/overriding-method-comments.mm?rev=165643&view=auto >> ============================================================================== >> --- cfe/trunk/test/Index/overriding-method-comments.mm (added) >> +++ cfe/trunk/test/Index/overriding-method-comments.mm Wed Oct 10 13:34:52 >> 2012 >> @@ -0,0 +1,111 @@ >> +// 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 < %t/out > > Nice, thanks! > >> +// rdar://12378793 > > A comment about what this tests would be more useful than a radar number, > since not everyone has access to radar. > > - Doug > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
