[cfe-commits] r165643 - in /cfe/trunk: include/clang/AST/Comment.h include/clang/AST/DeclObjC.h lib/AST/ASTContext.cpp lib/AST/Comment.cpp lib/AST/CommentDumper.cpp lib/AST/CommentSema.cpp lib/AST/DeclObjC.cpp test/Index/overriding-method-comments.mm tools/libclang/CXComment.cpp unittests/AST/CommentParser.cpp
jahanian
fjahanian at apple.com
Thu Oct 11 17:01:13 PDT 2012
This is mostly done in r165771. Please see bellow for couple of comments.
- fariborz
On Oct 11, 2012, at 11:44 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Oct 10, 2012, at 11:34 AM, Fariborz Jahanian <fjahanian at apple.com> 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
>
More information about the cfe-commits
mailing list