[llvm-branch-commits] [mlir] release/18.x: [mlir][NFC] Apply rule of five to *Pass classes (#80998) (PR #83971)
andrey-golubev wrote: ping @joker-eph: do you mind (back)porting this change to 18.x? if yes, let's close this, alternatively, please approve / merge, thank you! https://github.com/llvm/llvm-project/pull/83971 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] release/18.x: [mlir][NFC] Apply rule of five to *Pass classes (#80998) (PR #83971)
andrey-golubev wrote: > > Do we have any ABI stability guarantees for LLVM C++ APIs? I would think > > that a lot of back ports can break the C++ ABI of LLVM in general? > > Yes, we always try to keep the C++ API stable in point releases. Fair enough. Let's not backport it then. btw, > I think this will change the ABI. Do we have a policy on this for MLIR? I'm curious why do you think so, maybe I'm missing something. in any case, I'd say, strictly speaking, this change is "source-incompatible" (though unlikely anyone would actually encounter a failure). https://github.com/llvm/llvm-project/pull/83971 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] release/18.x: [mlir][NFC] Apply rule of five to *Pass classes (#80998) (PR #83971)
andrey-golubev wrote: > I think it's an ABI change (but I could be wrong), because some of the > implicit constructors are being deleted. What happens if a user of the > library was using these deleted constructors? I see your point. In this specific case, the special member functions were implicitly deleted anyway: the class used to have an explicit user-specified (non-default) copy ctor and that's about it. There is at least 1 member field in `Pass` that is non-movable and non-copyable. No copy-assignment operator -> implicitly deleted. No move semantics: move ctor would've dispatched to copy ctor, move assignment - implicitly deleted also. So the only case we've sacrificed with this patch is `X x(X())` (and other cases that call move ctor) - they are now rejected by the compiler instead of silently succeeding. Also, the users, I believe, are only within the inheritance chain, because all of this special member functions (except dtor) are under `protected` access specifier (unless there's some user lib that changes this at some lower level?). In any case, if there's already a released 18.0 then this change would only get into some other version and, as you mentioned, this is something you prefer to avoid for API/ABI incompatible changes. https://github.com/llvm/llvm-project/pull/83971 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] release/18.x: [mlir][NFC] Apply rule of five to *Pass classes (#80998) (PR #83971)
andrey-golubev wrote: Aha, so, apparently, I cannot even close a PR (lack of commit write access) :/ Could someone close this one as we don't plan on merging it? Not urgent anyway. https://github.com/llvm/llvm-project/pull/83971 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] release/18.x: [ODS][NFC] Cast range.size() to int32_t in accumulation (#85629) (PR #86677)
andrey-golubev wrote: > Hi @EugeneZelenko (or anyone else). If you would like to add a note about > this fix in the release notes (completely optional). Please reply to this > comment with a one or two sentence description of the fix. hi! @tstellar I don't think it's worth it: this is a tiny narrowing conversion warnings suppression (by adding explicit static cast), so not worth it to make it into release notes. https://github.com/llvm/llvm-project/pull/86677 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] PR for llvm/llvm-project#79600 (PR #79603)
andrey-golubev wrote: LGTM. But surely we need someone with commit access (sigh). CC @Dinistro @zero9178 https://github.com/llvm/llvm-project/pull/79603 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] PR for llvm/llvm-project#79600 (PR #79603)
andrey-golubev wrote: > Here we have to wait for the build bots, as they are mandatory. Finished, so asking for someone to press the merge button. Thanks in advance! https://github.com/llvm/llvm-project/pull/79603 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
