rjmccall added inline comments.
================
Comment at: clang/lib/CodeGen/CGClass.cpp:2016
CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
- /*Delegating=*/false, addr);
+ /*Delegating=*/false, addr, type);
}
----------------
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.
================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:117-118
+ llvm::Type *NewType = CGM.getTypes().ConvertType(DstTy);
+ This = getTargetHooks().performAddrSpaceCast(*this, This, SrcAS, DstAS,
+ NewType);
+ }
----------------
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.
================
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.
----------------
mantognini wrote:
> This is the fix for the first point in the description.
Can we unify this with the similar check we do elsewhere?
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
