rjmccall added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:13122 + if (Base->isVirtual()) { + BaseAlignment = Ctx.getTypeAlignInChars(Base->getType()); + Offset = CharUnits::Zero(); ---------------- 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. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13140 + E = E->IgnoreParens(); + switch (E->getStmtClass()) { + default: ---------------- You should add a case for unary `*`. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13211 + } + return Optional<std::pair<CharUnits, CharUnits>>(); +} ---------------- There's an `llvm::None` which has this effect. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13225 + return getBaseAlignmentAndOffsetFromPtr(cast<CastExpr>(E)->getSubExpr(), + Ctx); + case Stmt::UnaryOperatorClass: { ---------------- I don't think we guarantee that these will be no-op casts; all the explicit cast nodes should be handled the same as ImplicitCastExpr, in both of these functions. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13254 + BinaryOperatorKind Kind) { + auto LHSRes = getBaseAlignmentAndOffsetFromPtr(LHS, Ctx); + if (!LHSRes) { ---------------- Can you do this with just a type analysis on the operands instead of eagerly doing these calls? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13266 + if (!RHS->isIntegerConstantExpr(RHSRes, Ctx)) + return Optional<std::pair<CharUnits, CharUnits>>(); + CharUnits Offset = LHSRes->second; ---------------- This should be handled the same as array subscripting. Maybe you can extract a helper for that that's given a pointer expression, an integer expression, and a flag indicating whether it's a subtraction? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13279 + return HandleBinOp(BO->getLHS(), BO->getRHS(), Opcode); + break; + } ---------------- You should look through comma expressions. 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