[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]

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

> On Thu, Oct 18, 2012 at 7:18 PM, Douglas Gregor <dgregor at apple.com> wrote:
>> On Oct 18, 2012, at 5:03 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>>> 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.
> 
> I see now, thank you for the explanation.  Maybe improving comments or
> renaming CommentDecl/CurrentDecl could help to make this clear.
> 
>> 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 agree.
> 
>> I think we should also deal with rewriting of \p. Yes, it means some lookup, but it'll give a more consistent experience.
> 
> My main concern is not the additional lookup (it is even useful -- we
> could catch more "bugs" in comments), but the fact that there are
> unannotated references to parameters, and sometimes both annotated and
> unannotated references in the same comment (some will get rewritten,
> some will not).

It's a very reasonable concern. Personally, I'm more interested in people being able to do the right thing (mark their parameters with \p), so they'll get proper documentation across all redeclarations/overrides even if the names have changed.

	- Doug