PATCH: update RecursiveASTVisitor to visit attributes.

[Aaron Ballman]

On Fri, Dec 27, 2013 at 1:50 PM, Delesley Hutchins <delesley at google.com> wrote:
> Thanks for the review!  New patch enclosed, now tested so that it
> actually works.  :-)
>
> I notice that we actually have two versions of RecursiveASTVisitor.h;
> the second is in tools/libclang.  There are non-trivial differences
> between the two, and this is a parallel maintenance problem, as the
> two implementations are drifting apart.  What is the rationale for
> having two versions?  Should I update both?  The test code doesn't
> test the second version.
>
>> Would it make sense for this to take a const pointer? Or perhaps even
>> const reference?
>
> Most of the other routines take non-const pointers, so I think this is
> consistent with the rest of the API.
>
>> This would probably be better served by using an attr_iterator via
>> attr_begin/attr_end; it would then fix an assert crash which will
>> happen if the Decl has no attributes attached to it.
>
> Nice catch!  Done.
>
>>> +    if (!R.getValueAsBit("ASTNode"))
>>> +      continue;
>>
>> Do we want to filter out any statement or type attributes?
>
> Ideally, we want RecursiveASTVisitor to visit every attribute in the
> AST.  This patch only handles attributes on declarations thus far, but
> tablegen should emit code for all of them; the visitor can then be
> easily updated in subsequent patches.  ASTNode seems to be used as a
> filter elsewhere.   Would you suggest something else?

ASTNode is a reasonable filter here since anything without an AST node
hardly needs to be dealt with in an AST visitor. ;-) I was merely
wondering about filtering out statement and type attributes since
everything about this patch deals with decls.

We do have a slight problem (not with this patch) in that type
attributes tend to get parsed, but do not generate an AST node. So
there's no way for an AST visitor to ever find out that a type
attribute existed.

>> Spurious curly braces; but I'm wondering whether this for loop is even
>> required. Couldn't you accomplish the same thing without the Args
>> vector by just calling writeASTVisitorTraversal after creating the
>> Argument object?
>
> Done.  This loop structure has been cut-and-pasted throughout the
> file, but you're right -- it doesn't make too much sense here.
>
>> On a side note, I just realized we're leaking those Argument objects
>> all over the place. Oops.
>
> True.  But it doesn't matter, since this code is only run once during
> compile.  Every function in the file leaks the arguments.  :-)

Definitely doesn't matter for this patch. I tried to fix up the leaks
by using an OwningPtr, but since the nodes need to live in a vector on
occasion, and we don't have C++11 support quite yet, that patch will
have to wait for another day. :-P

Everything in your patch LGTM!

~Aaron