[PATCH] D139439: [Alignment][NFC] Use Align in MCStreamer emitZeroFill/emitLocalCommonSymbol

Guillaume Chatelet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 7 06:09:02 PST 2022


gchatelet added a comment.

In D139439#3977991 <https://reviews.llvm.org/D139439#3977991>, @courbet wrote:

> I don;t think we're guaranteed that there does not exist an override that accepts `0` as a value. What about switching to `MaybeAligned` instead ? We can always tighten in a second step once there are no other raw values left ?

It is quite painful to change virtual methods exactly because we can't know all the implementations. Private implementations need to adapt to the new signature, it adds churn.
If we go with `MaybeAlign` we will pay the price of that change twice. Once now and a second time later on when we decide to use `Align`.
I believe we'll never be in a position where it is safe to assume that implementers are not using `0` to convey something special.

I think it's better to go with this change and revert if it breaks someone. This way we learn something (and we can also fix the downstream code if it passes 0 for a wrong reason).
I'll remove the NFC part of the patch description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139439



More information about the llvm-commits mailing list