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