rjmccall added inline comments.
================
Comment at: lib/CodeGen/CGClass.cpp:1135
MemberExpr *ME2 = dyn_cast<MemberExpr>(RHS);
- if (dyn_cast<FieldDecl>(ME2->getMemberDecl()) != Field)
+ if (!ME2 || dyn_cast<FieldDecl>(ME2->getMemberDecl()) != Field)
return nullptr;
----------------
wristow wrote:
> rjmccall wrote:
> > I would prefer:
> >
> > if (MemberExpr *ME2 = dyn_cast<MemberExpr>(RHS)) {
> > if (ME2->getMemberDecl() == Field)
> > return Field;
> > }
> > return nullptr;
> I see that change removes the `dyn_cast<FieldDecl>`. Was that intended, or
> an oversight?
>
> In terms of changing the code-structure, in code on it's own, I do like the
> approach you described. But in this case, there is a sequence of `if
> (<condition1>) return nullptr; ... if (conditionN) return nullptr; return
> Field;`. Then after the block containing that set of guarded `nullptr`
> returns with a final `return Field;`, there is a similar block. And then
> there is a third block with a similar set. So changing the structure in that
> way breaks that pattern. With that in mind, do you still want that change
> done?
The dyn_cast has no effect. There is no situation in which the declarations
would compare equal without it where they would not with it, because Field is
already known to be a FieldDecl.
The structure of the existing code is unlikely to stay the same. Actually,
that code is quite worrying — it's making a lot of assumptions about how Sema
synthesizes defaulted assignment operator bodies. But I didn't want to ask you
to fix it when it's not the subject of your bug.
https://reviews.llvm.org/D29208
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
