mantognini marked 10 inline comments as done.
mantognini added a comment.
Mind the fact that I've rebased the changes onto a more recent master version.
If you look at the diff of v1 against v2 you might see some unrelated changes.
Let me know if there's anything else that I need to change.
================
Comment at: clang/lib/CodeGen/CGClass.cpp:2016
CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
- /*Delegating=*/false, addr);
+ /*Delegating=*/false, addr, type);
}
----------------
rjmccall wrote:
> mantognini wrote:
> > This is the only place where the new parameter has an interesting value. In
> > the ~10 other calls to `EmitCXXDestructorCall` overloads, the default value
> > of this new parameter is used instead.
> Arguments that are potentially required for correctness — as opposed to just
> enabling some optimization — should generally not have defaults. I think you
> should remove these defaults and require a sensible type to be passed down in
> all cases.
I've addressed that. I had to add some extra parameter/attributes here and
there. Please let me know if anything is can be improved.
================
Comment at: clang/lib/CodeGen/CGClass.cpp:1447
+ if (HaveInsertPoint()) {
+ // FIXME: Determine the type of the object being destroyed.
+ QualType ThisTy;
----------------
I'm not familiar enough with CXXCtorType and such, and would welcome some help
for these two FIXMEs.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:103
+ // we ensure a cast is added where necessary.
+ if (ThisTy.isNull()) {
+#ifndef NDEBUG
----------------
Despite no longer having a default parameter, not all call site can provide a
meaningful value ATM. That is why this check is still required.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:96
+ llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE,
+ QualType ThisTy) {
+ const CXXMethodDecl *DtorDecl = cast<CXXMethodDecl>(Dtor.getDecl());
----------------
mantognini wrote:
> This new parameter is required in order to call `performAddrSpaceCast`.
Now that it's no longer a default parameter. I've group ThisTy with This.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:117-118
+ llvm::Type *NewType = CGM.getTypes().ConvertType(DstTy);
+ This = getTargetHooks().performAddrSpaceCast(*this, This, SrcAS, DstAS,
+ NewType);
+ }
----------------
rjmccall wrote:
> Anastasia wrote:
> > mantognini wrote:
> > > This is effectively the fix for the third point mentioned in the
> > > description.
> > >
> > > Alternatively, `This = Builder.CreatePointerBitCastOrAddrSpaceCast(This,
> > > NewType);` seems to work equally well and does not require the extra new
> > > parameter.
> > Yes, I agree just using `CreatePointerBitCastOrAddrSpaceCast` would be
> > easier.
> >
> > As far as I remember (@rjmccall can correct me if I am wrong)
> > `performAddrSpaceCast` was added to workaround the fact that `nullptr`
> > constant might not be value 0 for some address spaces. However, considering
> > that it's not the first place where some big change has to be done in
> > CodeGen to be able to use the new API I am wondering if it's worth looking
> > at moving this logic to LLVM lowering phases that can map `nullptr`
> > constant into some arbitrary value. I think the challenge is to find where
> > LLVM assumes that `nullptr` is always 0. That might not be an easy task.
> >
> > PS, I am not suggesting to do it for this patch but just an idea to
> > investigate in the future.
> I continue to think that it makes sense to set Clang up to be able to handle
> language-level address spaces that don't exist at the level of LLVM IR.
Alright, I've kept it.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8428-8439
+ bool DiagOccured = false;
FTI.MethodQualifiers->forEachQualifier(
[&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
+ // This diagnostic should be emitted on any qualifier except an addr
+ // space qualifier. However, forEachQualifier currently doesn't visit
+ // addr space qualifiers, so there's no way to write this condition
+ // right now; we just diagnose on everything.
----------------
rjmccall wrote:
> mantognini wrote:
> > This is the fix for the first point in the description.
> Can we unify this with the similar check we do elsewhere?
Done.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8197
+ const DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
+ if (FTI.hasMethodTypeQualifiers() && !D.isInvalidType()) {
+ bool DiagOccured = false;
----------------
I've refactored that bit out of the two mentioned functions. Mind the fact that
it now always check `!D.isInvalidType()`. I think it makes sense, but let me
know if that's not the case.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8200
+ FTI.MethodQualifiers->forEachQualifier(
+ [DiagID, &S, &DiagOccured](DeclSpec::TQ, StringRef QualName,
+ SourceLocation SL) {
----------------
I've reduced the capture group to the bare minimum. Hopefully, this is
consistent with LLVM style. But let me know if I should change this back to
`[&]`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64569/new/
https://reviews.llvm.org/D64569
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
