DavidSpickett accepted this revision. DavidSpickett added a comment. This LGTM.
================ Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:58 + uint32_t reg_num) override; + lldb::addr_t ReadPC(bool *success); + bool WritePC(lldb::addr_t pc); ---------------- seehearfeel wrote: > lh03061238 wrote: > > DavidSpickett wrote: > > > I think the older targets use this form but for riscv they went with > > > `llvm::Optional<lldb::addr_t> ReadPC();` which I prefer over pointer plus > > > addr_t. > > > I think the older targets use this form but for riscv they went with > > > `llvm::Optional<lldb::addr_t> ReadPC();` which I prefer over pointer plus > > > addr_t. > > > > EmulateInstructionLoongArch is relatively simple Compared with riscv. If > > use llvm::Optional<lldb::addr_t> ReadPC(), There will be more type > > conversions here. I prefer to keep that definition for now, Considering the > > complexity of the code at this stage. > > > > Thank you. > @DavidSpickett Are you OK with the current code? > I prefer to keep that definition for now, Considering the complexity of the > code at this stage. Totally fine, this is a lot of duplicating existing code to begin with so I appreciate why you'd keep the differences minimal. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139158/new/ https://reviews.llvm.org/D139158 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
