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

[Philip Craig]

Ping.

On Fri, Sep 7, 2012 at 7:32 PM, Manuel Klimek <klimek at google.com> wrote:
> +richard & james, who might both have things to say about the
> semantics involved...
>
>
> On Tue, Sep 4, 2012 at 11:43 AM, Philip Craig <philipjcraig at gmail.com> wrote:
>> 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.