[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
Douglas Gregor
dgregor at apple.com
Thu Oct 11 11:44:40 PDT 2012
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.
> 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?
> 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