[PATCH] D101588: [M68k] fix -Wdefaulted-function-deleted and -Woverloaded-virtual

Min-Yih Hsu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 28 09:50:52 PDT 2021


myhsu added a comment.

In D101588#2771467 <https://reviews.llvm.org/D101588#2771467>, @dblaikie wrote:

> In D101588#2771414 <https://reviews.llvm.org/D101588#2771414>, @myhsu wrote:
>
>> In D101588#2769214 <https://reviews.llvm.org/D101588#2769214>, @dblaikie wrote:
>>
>>> In D101588#2767764 <https://reviews.llvm.org/D101588#2767764>, @myhsu wrote:
>>>
>>>> In D101588#2764656 <https://reviews.llvm.org/D101588#2764656>, @dblaikie wrote:
>>>>
>>>>> In D101588#2730131 <https://reviews.llvm.org/D101588#2730131>, @dblaikie wrote:
>>>>>
>>>>>> @myhsu (or someone else with M68k familiarity) - the virtual function fix here is a semantic change (previously whatever was calling the virtual function would not have been getting the implementation it is now getting). Is there any testing that could be added to demonstrate that this semantic change is correct/avoid the regression/test the codepath that is missing coverage?
>>>>>
>>>>> @myhsu Ping on this question ^
>>>>
>>>> This might not be the right place to ask, but I accidentally found that no one outside `lib/Target` is actually using `emitCalleeSavedFrameMoves` with 4 arguments (please correct me if I'm wrong. I only found one call site of `emitCalleeSavedFrameMoves` with 2 arguments). Combing with the diff presented here, this patch is actually a NFC one. So I don't think adding a test will be necessary here.
>>>> A spin-off question might be: Why was `emitCalleeSavedFrameMoves` with 4 arguments got exposed as virtual function at the first place if no one is using it?
>>>
>>> Could you check the version control history of the function? Perhaps it was added for some use at some point, but that use is no longer needed and it could now be removed?
>>
>> It was first introduced in https://reviews.llvm.org/D79978.
>
> Ah, thanks for the pointer!
>
>> `emitCalleeSavedFrameMoves` with 4 arguments was not used outside `lib/Target` in that particular patch and I doubt anyone (lib/CodeGen) has used it ever since then.
>> I can send a patch regarding this matter if that's appropriate.
>
> Yep, sounds good to me. Please do!

Sorry for the delay, I'd just submitted https://reviews.llvm.org/D103328


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101588



More information about the llvm-commits mailing list