Conversation
Added for loop for readability, changed a couple of things and confirmed that it builds successfully and works as intended, minor update
stuxnot
left a comment
stuxnot
left a comment
There was a problem hiding this comment.
Thank you very much for the PR. Before merging it, I would need you to rebase it on master, incorporate the requested changes, and improve the commit message to make it clear what the actual changes are. Also, your current changes introduce a bug. You can reproduce it by running the example binary after building, or running make/ninja test in the build directory (depending on the build tool you are using).
I've added the loop you suggested and refactored some of the code for efficiency and readability.
|
I have tested this change in build/ directory using make test and it returns success, I've also used tool/format.sh to fix any formatting issues. |
There was a problem hiding this comment.
Thank you for making the changes I requested. I've reviewed the new changes and still have some small nitpicks.
Also regarding the commit history: We are trying to keep the commit history of the main branch relatively clean and clear. When I asked you to improve the commit description and rebase on main, I meant to use git rebase to rewrite the branch history to use the current main as the basis and to actually rewrite the commits so that the messages are descriptive. This would allow us to merge your changes without squashing the commit history.
One last thing, please do not write something like "improves efficiency" into the commit messages if you have no metric to back up this claim. If you want to keep such a message, please show some evidence that the changes had a measurable and reproducible impact on the performance of Nyxstone.
This is all minor stuff, just wanted to let you know. If you are not sure how to rewrite the commit history - don't worry. I'll just squash your commits and add a short merge commit description of my own, since your changes should fit a single commit. If you have any questions, feel free to ask :)
| Instruction new_insn{address + pos, insn_str, {}}; | ||
| new_insn.bytes.assign(data.begin() + pos, data.begin() + pos + insn_size); | ||
| instructions->push_back(std::move(new_insn)); |
There was a problem hiding this comment.
If you are touching this code anyway, in my opinion doing
auto insn_bytes = data.slice(pos, insn_size);
Instruction new_insn { address + pos, insn_str, std::move(insn_bytes) };
instructions->push_back(std::move(new_insn));is a bit clearer.
| << " / address " << std::hex << address + pos; | ||
| if (!error_msg.empty()) { | ||
| error_stream << "(= " << error_msg.c_str() << " )"; | ||
| error_stream << " (= " << error_msg.c_str() << ")"; |
There was a problem hiding this comment.
I don't see a reason to touch the whitespaces in the error string (at least in this PR), especially since we are planning to rewrite the error handling anyway. Please revert this change.
|
|
||
| // We exit either if we reached the end of the provided bytes, or if we have disassembled as many instructions | ||
| // as the user has requested | ||
| while (pos < data.size() && (disassemble_all || insn_count < count)) { |
There was a problem hiding this comment.
You re-added the conditions that you removed earlier from the end of this loop, i.e. lines 462-464 and 467-469. These conditions are redundant because of the conditions in the while loop. Please remove them.
| uint64_t pos = 0; | ||
| uint64_t insn_count = 0; |
There was a problem hiding this comment.
Please leave these two in seperate lines. We try to use this style of variable initialization as the single line initialization can easily lead to forgetting to assign a value to a variable.
2 participants
Added for loop for readability, changed a couple of things and confirmed that it builds successfully and works as intended, minor update