Ping.
On Fri, Sep 7, 2012 at 7:32 PM, Manuel Klimek <[email protected]> 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 <[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
