[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

[Douglas Gregor]

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.

>> 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?

	- Doug