rtrieu added a comment. Next time, add
> Differential Revision: <URL> to your commit and Phabricator will close the diff automatically. http://llvm.org/docs/Phabricator.html ================ Comment at: lib/Analysis/CFG.cpp:99-104 @@ +98,8 @@ + // Currently we're only given EnumConstantDecls or IntegerLiterals + auto *C1 = cast<EnumConstantDecl>(cast<DeclRefExpr>(A)->getDecl()); + auto *C2 = cast<EnumConstantDecl>(cast<DeclRefExpr>(B)->getDecl()); + + const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext()); + const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext()); + return E1 == E2; +} ---------------- george.burgess.iv wrote: > rtrieu wrote: > > There's a few extra casts in here, plus some blind conversions between > > types. Add your assumptions for the types in asserts. Also, DeclContext > > should use cast<> to get to Decl types. I recommend the following: > > > > ``` > > assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2)); > > auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl(); > > auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl(); > > > > assert(isa<EnumConstantDecl>(Decl1) && isa<EnumConstantDecl>(Decl2)); > > const DeclContext *DC1 = Decl1->getDeclContext(); > > const DeclContext *DC2 = Decl2->getDeclContext(); > > > > assert(isa<EnumDecl>(DC1) && isa<EnumDecl>(DC2)); > > return DC1 == DC2; > > > > ``` > I was under the impression that the `cast<Foo>(Bar)` asserts `isa<Foo>(Bar)` > for me, so I thought that asserts like those would just be redundant. Changed > to your version anyway :) You are correct, 'cast<Foo>(Bar)' does assert 'isa<Foo>(Bar)'. However, when Bar is not Foo, using the assert here means the crash will produce a backtrace will point straight to this function instead of an assert that points deep into the casting functions. http://reviews.llvm.org/D13157 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
