rjmccall added a comment.
Other changes look good to me, thanks.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13122
+ if (Base->isVirtual()) {
+ BaseAlignment = Ctx.getTypeAlignInChars(Base->getType());
+ Offset = CharUnits::Zero();
----------------
ahatanak wrote:
> rjmccall wrote:
> > Oh, this — and all the other places that do presumed alignment based on a
> > pointee type — needs a special case for C++ records with virtual bases,
> > because you need to get its presumed alignment as a base sub-object, not
> > its presumed alignment as a complete object, which is what
> > `getTypeAlignInChars` will return. The right way to merge that information
> > is to get the normal alignment — which may be lower than expected if
> > there's a typedef in play — and then `min` that with the base sub-object
> > alignment.
> I think the base sub-object alignment in this case is the
> `NonVirtualAlignment` of the class (the `CXXRecordDecl` of `Base` in this
> function), but it's not clear to me what the normal alignment is. I don't
> think it's what `Ctx.getTypeAlignInChars(Base->getType())` returns, is it? I
> see `CodeGenModule::getDynamicOffsetAlignment` seems to be doing what you are
> suggesting, but I'm not sure whether that's what we should be doing here. It
> looks like it's comparing the alignment of the derived class and the
> non-virtual alignment of the base class.
The base sub-object alignment is the class's non-virtual alignment, right.
By the normal alignment, I mean the alignment you're starting from for the
outer object — if that's less than the base's alignment, the base may be
misaligned as well. For the non-base-conversion case, that's probably not
necessary to consider.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78767/new/
https://reviews.llvm.org/D78767
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
