Page MenuHomePhabricator

[Alignment] Replace commonAlignment with std::min
ClosedPublic

Authored by gchatelet on Jun 22 2022, 8:07 AM.

Details

Summary

commonAlignment is a shortcut to pick the smallest of two Align
objects. As-is it doesn't bring much value compared to std::min.

Diff Detail

Event Timeline

gchatelet created this revision.Jun 22 2022, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 8:07 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
gchatelet requested review of this revision.Jun 22 2022, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 8:07 AM

I've added the authors of the code as reviewers so we can discuss if std::min makes sense or not

@aemerson @bkramer is std::min(Align, Align) clear enough?

@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?

@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 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?

bkramer accepted this revision.Jun 27 2022, 9:21 AM

I think given the semantic difference removing just the one overload makes sense.

This revision is now accepted and ready to land.Jun 27 2022, 9:21 AM
This revision was landed with ongoing or failed builds.Jun 28 2022, 12:15 AM
This revision was automatically updated to reflect the committed changes.

Thx for the review!