This is patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790
Details
- Reviewers
courbet - Commits
- rG1b2842bf902a: [Alignment][NFC] CreateMemSet use MaybeAlign
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
2910 | The argument can be 0 and in that case assumeAligned will turn it into 1. |
@gchatelet in general would it be possible to make changes like this in a backwards-compatible way, or in two stages without a "flag day" change? We have out-of-tree users of CreateMemSet and it's awkward to change them all at exactly the same time as we merge in this change from upstream llvm, and we have had the same problem with other MaybeAlign changes recently. I realise that LLVM doesn't make any official promises about API stability.
Thx for letting me know @foad . I'll make sure to keep the old API with a deprecation message from now on.
Do you have any other suggestions on how to make this less painful for out-of-tree users? I'm afraid that the cleanup phase (removal of deprecated function) will be disruptive as well.
Removing deprecated functions is generally OK, as long as it happens *after* the preferred function is introduced, so we have time to switch over.
In the specific case of functions taking Align instead of unsigned, perhaps you could start off allowing implicit conversion from unsigned to Align and then remove it later, when all callers have been updated? Or perhaps it's too late for that now.
Thx for letting me know @foad . I'll make sure to keep the old API with a deprecation message from now on.
Do you have any other suggestions on how to make this less painful for out-of-tree users? I'm afraid that the cleanup phase (removal of deprecated function) will be disruptive as well.Removing deprecated functions is generally OK, as long as it happens *after* the preferred function is introduced, so we have time to switch over.
Sure. My apologies for this.
In the specific case of functions taking Align instead of unsigned, perhaps you could start off allowing implicit conversion from unsigned to Align and then remove it later, when all callers have been updated? Or perhaps it's too late for that now.
I tried but it's not convenient for me to find the call sites that need updating. Also I don't want to introduce regressions by having automatic conversion from bool or anything implicitly convertible to int. By proceeding like this, I'm making sure that each change is reviewed and that the semantic is correct. When in doubt I try to get a review from the original author.
@foad do you have any insights on how to go with the deprecation?
LLVM has this LLVM_ATTRIBUTE_DEPRECATED macro, it's convenient to get a warning but it only works when building without -Wall.
Bots and in-tree users have it set by default, it's fine as I'll be fixing those but how about out of tree users?
Another option would be to have a comment but then nobody will actually update the code before it breaks.
Did you mean to write _with_ -Wall? I fail to see anything that stops the macro from working with -Wall?
Generally, I'd say using that macro a bit more for this kind of thing would be a good idea.
Sorry I meant -Werror which will turn warnings into errors, then deprecation messages will break compilation.
I thought it was ON by default but it's not the case.
I'll go this route then. Thx!
const ?