[PATCH] D64790: [LLVM][NFC] Adding an Alignment type to LLVM

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: llvm/include/llvm/Support/Alignment.h:21
+// `undefined alignment requirements` and 1 meaning `no alignment requirements`,
+// this caused hard to spot bugs:
+//
----------------
I don't think the prior history is useful here.


================
Comment at: llvm/include/llvm/Support/Alignment.h:73
+  // Default is byte-aligned.
+  Align() : ShiftValue(0) {}
+  // Do not perform checks in case of copy/move construct/assign, because the
----------------
You can remove the ctor and instead do `uint8_t ShiftValue = 0;` above.


================
Comment at: llvm/include/llvm/Support/Alignment.h:99
+// undefined (0) alignment.
+struct MaybeAlign final {
+private:
----------------
Why is this better than `using MaybeAligned = Optional<Align>`?
Do we care about packing that extra bit?
It also seems that `Align` can have a shift of `255` and this can't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64790