[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