rjmccall added inline comments.
================
Comment at: clang/include/clang/AST/DeclCXX.h:784
+ return !hasDefinition() || !isDynamicClass() || hasAnyDependentBases();
+ }
+
----------------
Both of these new methods deserve doc comments explaining that they're
conservative checks because the class might be incomplete or dependent.
I think `NonDynamic` would read better than `NotDynamic`.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623
+ const CXXRecordDecl *SourceClassDecl =
+ E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+ const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl();
----------------
Unnecessary `getTypePtr()`.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1624
+ E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+ const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl();
+
----------------
The opposite of `Dst` is `Src`. Alternatively, the opposite of `Source` is
`Destination` (or `Result`). Please pick.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1633
+ bool DstMayNotBeDynamic =
+ !DstClassDecl || DstClassDecl->mayBeNotDynamicClass();
+ if (SourceMayBeNotDynamic && DstMayBeDynamic) {
----------------
If you made a couple of tiny helper functions here that you could invoke on
either `SourceClassDecl` or `DstClassDecl`, you could avoid some redundant
logic and also make the calls self-documenting enough to legibly inline into
the `if`-conditions.
...in fact, since you start from a QualType in every case, maybe you should
just define the helper as a method there.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647
+ }
+ }
+
----------------
Incidentally, how do you protect against code like this?
A *ptr;
reinterpret_cast<B *&>(ptr) = new B();
ptr->foo();
Presumably there needs to be a launder/strip here, but I guess it would have to
be introduced by the middle-end when forwarding the store? The way I've
written this is an aliasing violation, but (1) I assume your pass isn't
disabled whenever strict-aliasing is disabled and (2) you can do this with a
memcpy and still pretty reliably expect that LLVM will be able to eventually
forward the store.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1791
+ // dynamic information from invariant.group.
+ if (DstClassDecl && DstClassDecl->mayBeDynamicClass())
+ IntToPtr = Builder.CreateLaunderInvariantGroup(IntToPtr);
----------------
Another place you could definitely just use that helper function on QualType.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1802
+ const CXXRecordDecl *ClassDecl =
+ E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+ // Casting to integer requires stripping dynamic information as it does
----------------
Same.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3305
+ if (RD->mayBeDynamicClass())
+ RHS = Builder.CreateStripInvariantGroup(RHS);
+ }
----------------
Yeah, helper function on QualType, please. :)
Repository:
rL LLVM
https://reviews.llvm.org/D47299
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
