rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.
LGTM.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13122
+ if (Base->isVirtual()) {
+ BaseAlignment = Ctx.getTypeAlignInChars(Base->getType());
+ Offset = CharUnits::Zero();
----------------
ahatanak wrote:
> rjmccall wrote:
> > 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.
> >
> Let me know if the comment explaining why we are taking the minimum makes
> sense. I added a test case for this too (`&t10`).
Looks good.
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
