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.


cfe-commits mailing list

Reply via email to