[PATCH] D78767: [Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

[John McCall via Phabricator via cfe-commits]

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