[cfe-commits] r166130 - in /cfe/trunk: bindings/xml/comment-xml-schema.rng include/clang/AST/Comment.h include/clang/AST/PrettyPrinter.h lib/AST/Comment.cpp lib/AST/DeclPrinter.cpp test/Index/annotate-comments-availability-attrs.cpp test/Index/an

[jahanian]

On Oct 18, 2012, at 9:22 AM, Douglas Gregor <dgregor at apple.com> wrote:

> 
> On Oct 18, 2012, at 4:42 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
> 
>> On Thu, Oct 18, 2012 at 12:58 AM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>>> Author: fjahanian
>>> Date: Wed Oct 17 16:58:03 2012
>>> New Revision: 166130
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=166130&view=rev
>>> Log:
>>> [Doc parsing]: This patch adds <Declaration> tag to
>>> XML comment for declarations which pretty-prints
>>> declaration. I had to XFAIL one test annotate-comments.cpp.
>>> This test is currently unmaintainable as written.
>>> Dmitri G., can you see what we can do about this test.
>> 
>> Hi Fariborz,
>> 
>> While I agree that annotate-comments is hard to maintain, but it is
>> not just "a test", it is *the* test (it tests almost all aspects of
>> comments parsing), and thus I disagree with your approach of
>> committing a new feature and XFAILing this test -- I think we should
>> have discussed this.
> 
> I fully agree with Dmitri. Revert, don't XFAIL, if a test is breaking.

I am working on this before I go out of town.

> 
>>> Modified: cfe/trunk/include/clang/AST/Comment.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Comment.h?rev=166130&r1=166129&r2=166130&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/AST/Comment.h (original)
>>> +++ cfe/trunk/include/clang/AST/Comment.h Wed Oct 17 16:58:03 2012
>>> @@ -905,9 +905,9 @@
>>>  /// Declaration the comment is attached to.  Should not be NULL.
>>>  const Decl *CommentDecl;
>>> 
>>> -  /// Location of this declaration. Not necessarily same as location of
>>> -  /// CommentDecl.
>>> -  SourceLocation Loc;
>>> +  /// CurrentDecl is the declaration for which comment is being put into an XML comment.
>>> +  /// Not necessarily same as CommentDecl.
>>> +  const Decl *CurrentDecl;
>> 
>> I think that this "CurrentDecl" variable represents (or should
>> represent) some bigger concept than what is stated in the comment.  It
>> should not be related with XML output at all.  And, I don't see (in
>> current code) how it can be different from CommentDecl.
> 
> I hope my other message explained it. However, the comment needs to be improved to describe this. Fariborz?

As you pointed out.CurrentDecl is the decl for which this comment is intended for. CommentDecl is the decl. for which
comment was written by the user. I will add more to the comment (no pun intended :).
- fariborz

> 
> 	- Doug
> 
>