[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:13066
+std::tuple<bool, CharUnits, CharUnits>
+static getBaseAlignmentAndOffsetFromLValue(const Expr *E, ASTContext &Ctx) {
+  E = E->IgnoreParens();
----------------
I think an `Optional<std::pair>` would be a more self-documenting type here, and you'd be able to test the result in an `if`.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13082
+                               std::get<2>(P) + EltSize * IdxRes.getExtValue());
+      }
+    }
----------------
You still need to conservatively adjust the alignment when the index isn't a constant expression.  You can do that by reducing the base result to a simple alignment, adding the array element size as an offset, and then reducing again.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13091
+      if (VD->getType()->isReferenceType())
+        return getBaseAlignmentAndOffsetFromLValue(VD->getInit(), Ctx);
+      return std::make_tuple(true, Ctx.getDeclAlign(VD), CharUnits::Zero());
----------------
Reference variables don't have to have initializers; e.g. they can be `extern`.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13111
+  }
+  }
+  return std::make_tuple(false, CharUnits::Zero(), CharUnits::Zero());
----------------
Derived-to-base conversions.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13160
+          Opcode == BO_Add)
+        HandleBinOp(BO->getRHS(), BO->getLHS(), Opcode, P);
+      return P;
----------------
This all seems like it'd be simpler if you just checked for the pointer/integer operands and then swapped if necessary.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13164
+    break;
+  }
+  }
----------------
Derived-to-base conversions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78767/new/

https://reviews.llvm.org/D78767