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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

Thx for the review @jfb. Anything else?



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


================
Comment at: llvm/include/llvm/Support/Alignment.h:99
+// undefined (0) alignment.
+struct MaybeAlign final {
+private:
----------------
jfb wrote:
> 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.
When the data type for `Align` was `unsigned` or `uint64_t`, `llvm::Optional` was costly in terms of storage
```
sizeof(llvm::Optional<unsigned>) == 8
sizeof(llvm::Optional<uint64_t>) == 16
```

But with Align using `uint8_t` I agree it's not a big deal anymore `sizeof(llvm::Optional<uint8_t>) == 2`

I've added a comment about `Align::ShiftValue` being less than 64 by construction.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64790