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

[Richard Smith]

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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120917/d03d441e/attachment.html>