[cfe-commits] [PATCH] RecursiveASTVisitor: fix type traversal for EnumDecl

[Philip Craig]

On Mon, Sep 3, 2012 at 9:30 PM, Manuel Klimek <klimek at google.com> wrote:
> On Mon, Sep 3, 2012 at 12:53 PM, Philip Craig <philipjcraig at gmail.com> wrote:
>> On Mon, Sep 3, 2012 at 8:14 PM, Manuel Klimek <klimek at google.com> wrote:
>>> On Sat, Sep 1, 2012 at 7:41 AM, Philip Craig <philipjcraig at gmail.com> wrote:
>>>> RecursiveASTVisitor was traversing D->getTypeForDecl() for EnumDecl,
>>>> but shouldn't (same as for other TagDecl). It also wasn't traversing
>>>> the C++11 integer type.
>>>
>>> Don't we still visit the type as part of the typeloc traversal after your patch?
>>
>> We still visit a type, but it's a different type. Previously it
>> visited an EnumType (which is the type that is the result of this
>> declaration, not part of it), but now it visits whatever type has been
>> specified, if any (such as BuiltinType for int). Maybe I should have
>> split this into separate patches?
>>
>>> Perhaps add a negative test for what we don't want to visit any more?
>>
>> I can if you think it's a good idea. I didn't for two reasons.
>> - There's no end of things you could test for the absence of, is this
>> important enough?
>
> Yes, the potential for negative tests is unlimited. On the other hand,
> having regression tests is in my opinion very useful and valuable - if
> one person has made the error once, chances are, somebody else will
> introduce the same error again (I've seen that happen many times).
>
>> - Most of the other TypeDecl are already skipping this type, and have
>> comments to that effect, but no tests. Should they have tests too?
>
> Well, the RAV is definitely undertested. But I don't think the
> asymmetry is too bad here - suddenly having to write tests for
> everything costs a lot of effort, but introducing tests when fixing
> bugs / implementing new features will already give lots of pay-out at
> comparatively little effort.
>
>> The way I'd prefer to test this is to visit everything, compare that
>> against a whitelist of things we expected to visit, and fail the test
>> if anything unexpected was visited, rather than having a blacklist of
>> things not to visit. That seems like it will need a lot of test
>> framework changes though.
>
> I like tests that test very specific things. In my experience those
> tend to be easier to maintain over the long run.

I'm going to split this patch into two. Here's the first patch, which
removes getTypeForDecl() traversal. I've removed it from
TemplateTypeParmDecl as well, and added tests for them. Once that is
okay I'll send the second patch, which adds EnumDecl integer type
traversal.

Patch description:
TypeDecl::getTypeForDecl() is always the type being defined by the
declaration, so it isn't written in the source and shouldn't be
traversed by RecursiveASTVisitor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: RAV-getTypeForDecl.patch
Type: application/octet-stream
Size: 3214 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120904/c5f2608a/attachment.obj>