[cfe-commits] r165771 - in /cfe/trunk: include/clang/AST/ASTContext.h include/clang/AST/Comment.h lib/AST/ASTContext.cpp lib/AST/Comment.cpp lib/AST/CommentSema.cpp test/Index/overriding-method-comments.mm tools/libclang/CXComment.cpp

[Douglas Gregor]

Hi Dmitri,

On Oct 18, 2012, at 5:03 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Fri, Oct 12, 2012 at 2:52 AM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>> +comments::FullComment *ASTContext::cloneFullComment(comments::FullComment *FC,
>> +                                                    const Decl *D) const {
>> +  comments::DeclInfo *ThisDeclInfo = new (*this) comments::DeclInfo;
>> +  ThisDeclInfo->CommentDecl = D;
>> +  ThisDeclInfo->IsFilled = false;
>> +  ThisDeclInfo->fill();
>> +  ThisDeclInfo->CommentDecl = FC->getDecl();
>> +  comments::FullComment *CFC =
>> +    new (*this) comments::FullComment(FC->getBlocks(),
>> +                                      ThisDeclInfo);
>> +  return CFC;
>> +
>> +}
> 
> Hi Fariborz,
> 
> I think that cloneFullComment() is evil -- it breaks some assumptions
> about DeclInfo and does not properly introduce a concept of a comment
> being reused for a redeclaration.
> 
> (1) DeclInfo is intended to be immutable once filled; here we are just
> replacing the original decl that provided all the information with
> some other declaration.  I see that it apparently did not break
> anything, but it is not the best solution.  Should we better keep two
> DeclInfos per FullComment?

It's weird to be overwriting CommentDecl in cloneFullComment(), but I don't think we need to have two DeclInfos per FullComment. The intent here is that CommentDecl is the declaration to which the comment was actually attached (in the source), and CurrentDecl would be the declaration with which the FullComment is associated. The information in the DeclInfo corresponds to CurrentDecl.

> (2) ParamCommandComment::getParamName and others used to return the
> parameter name *as written* in \param command, without any regard to
> resolving the name to an index in function parameter list.  Now they
> serve dual purpose of returning that parameter name or rewriting it.
> This also breaks the purpose of getParamNameRange() nearby.  I think
> that getParamName should be split into two getter functions -- to get
> parameter name as written (getParamName() as it was) or with some
> magic rewriting (getParamNameInDecl()).

I'm fine with splitting it into two functions. In general in the Clang ASTs, we have "getFoo()" be the semantic property and we add a "getFooAsWritten()" for the syntax as written. I suggest we do that.

I think we should also deal with rewriting of \p. Yes, it means some lookup, but it'll give a more consistent experience.

	- Doug