lh03061238 added a comment. In D140386#4007705 <https://reviews.llvm.org/D140386#4007705>, @DavidSpickett wrote:
> Please split this patch into 2: > > - the cleanup of the existing branch instructions > - the addition of the new ones > > The changes look good but let's keep each commit to doing 1 thing. > > You are missing tests though, and this is my mistake for not asking for them > in the previous patches. See https://reviews.llvm.org/D139294 for examples of > how riscv is doing it. It doesn't need to be totally comprehensive. For > example we can assume basic register operands are extracted correctly. So for > a branch the tests would be one where it is supposed to branch and one where > it isn't. The infrastructure for testing the emulation varies between arch > but as before, riscv probably the best reference point. > > Please add testing for the existing instructions as parent patches of this > one (these 2 once you've split them), then these new ones should have tests > too. > > Note: I will be off for Christmas break from tomorrow until the 2nd week of > January. So you can add any of the other common reviewers in lldb in the > meantime. Or wait for me to be back, feel free to pile up reviews :) Thanks, I will modify the content of this patch to add unit tests first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140386/new/ https://reviews.llvm.org/D140386 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
