On Thu, Oct 1, 2015 at 2:50 PM, George Burgess IV
<george.burgess...@gmail.com> wrote:
> george.burgess.iv closed this revision.
> george.burgess.iv marked 4 inline comments as done.
> george.burgess.iv added a comment.
>
> Changed code to address all feedback + committed as r249053. Thanks for the
> reviews!
>
>
> ================
> Comment at: lib/Analysis/CFG.cpp:49
> @@ +48,3 @@
> +tryNormalizeBinaryOperator(const BinaryOperator *B) {
> + auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
> + E = E->IgnoreParens();
> ----------------
> rtrieu wrote:
>> Why is this a lambda instead of a helper function?
> Because it's small + super specific to `tryNormalizeBinaryOperator`, and
> making it a lambda lets me scope it it only to where it's useful. It's now a
> helper function. :)
>
> ================
> 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;
> +}
> ----------------
> 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 :)
cast<> already does assert, so those new asserts are redundant and
should be removed. Sorry, I didn't catch that suggestion earlier. I
would recommend:
auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl();
auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl();
assert(isa<EnumConstantDecl>(Decl1) && isa<EnumConstantDecl>(Decl2));
return Decl1->getDeclContext() == Decl2->getDeclContext();
I don't think there's a point to asserting that an EnumConstantDecl
has a declaration context that is an EnumDecl -- if that weren't true,
we'd have broken long before reaching this code.
~Aaron
>
>
> http://reviews.llvm.org/D13157
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
