+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 <[email protected]> wrote: > On Fri, Sep 7, 2012 at 2:32 AM, Manuel Klimek <[email protected]> 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 <[email protected]> >> wrote: >> > On Mon, Sep 3, 2012 at 9:30 PM, Manuel Klimek <[email protected]> wrote: >> >> On Mon, Sep 3, 2012 at 12:53 PM, Philip Craig <[email protected]> >> >> wrote: >> >>> On Mon, Sep 3, 2012 at 8:14 PM, Manuel Klimek <[email protected]> >> >>> wrote: >> >>>> On Sat, Sep 1, 2012 at 7:41 AM, Philip Craig <[email protected]> >> >>>> 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. > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
