[PATCH] D128345: [Alignment] Replace commonAlignment with std::min

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

In D128345#3607978 <https://reviews.llvm.org/D128345#3607978>, @bkramer wrote:

> In D128345#3607911 <https://reviews.llvm.org/D128345#3607911>, @gchatelet wrote:
>
>> @aemerson @bkramer is `std::min(Align, Align)` clear enough?
>
> I think so. But I find it rather confusing to have the other overload of `commonAlign` still around. Are you planning to remove that?

I didn't plan to remove it. They convey slightly different semantics:

- `commonAlignment(Align, Align)` is the smallest of the alignments (they are both valid alignments by construction)
- `commonAlignment(Align, uint64_t)` or `MinAlign(uint64_t, uint64_t)`as defined in MathExtras.h <https://github.com/llvm/llvm-project/blob/f65c88c42fdd0e46d16fe31737e6627c56de77c3/llvm/include/llvm/Support/MathExtras.h#L697-L706> return the alignment that satisfies both `Lhs` and `Rhs`

The discrepancy came from the introduction of the type, before `Align` everything was working on integer types and used `MinAlign`.
Now we have a proper type and in this case picking the //common alignment// is the same as taking the minimum.
When we start with integer types we have to do more work. Note that these functions are also less safe as integer promotion can kick in, they will happily take an unsigned type as input.

What would you suggest? We can keep both functions if you think it makes sense, or find a better name for the remaining function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128345