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

[Manuel Klimek]

+djasper, lots of good questions raised by richard which are highly
relevant to the typeloc matcher design

On Mon, Sep 17, 2012 at 9:41 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Sep 7, 2012 at 2:32 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> +richard & james, who might both have things to say about the
>> semantics involved...
>
>
> We definitely do want to traverse the underlying type as written for an
> enum.
>
> For the other half of the patch (not visiting the type declared by enums and
> template type parameters), I'd like to understand what RAV's policy is here
> -- the original design [0] doesn't even talk about visiting Types, just
> TypeLocs. I think this patch is the right direction, but I think we need to
> know what the end goal is, too.
>
> From a quick skim of RAV, the cases where we currently switch from
> traversing a node with locations to traversing a node without locations
> (Type, NestedNameSpecifier, TemplateArgument) are:
>   1) Cases where we don't preserve source location information (for
> ComplexTypeLoc and VectorTypeLoc, the exception types in a dynamic exception
> specification for a FunctionProtoTypeLoc, and the NestedNameSpecifier in a
> TemplateName). These are bugs.
>   2) For the class type in a MemberPointerTypeLoc. This is a bug -- we have
> the TypeLoc but don't traverse it!
>   3) For the type declared in an EnumDecl or a TemplateTypeParmDecl. We
> probably shouldn't be visiting these at all (this is fixed by Philip's
> patch).
>   4) When the AST is missing the type source information (for a
> DeclaratorDecl or for a TemplateArgumentLoc for a type). I'm not sure
> whether there are any cases where this is reasonable for a valid AST.
>   5) For AutoType, we visit the deduced Type (and there is no TypeLoc). This
> is probably convenient for RAV users, but it's not clear whether it's
> correct (especially since we don't have a definition of 'correct'). We don't
> do the same for SubstTemplateTypeParmTypes.
>
> Perhaps the goal should be that traversing an entity with location
> information should never lead to traversing an entity without? What do RAV
> clients actually want here?
>
>  [0]: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-June/009273.html
>
>>
>> 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.
>
>