vabold / Kinoko

A reimplementation of Mario Kart Wii's physics engine in C++
MIT License
32 stars 9 forks source link

Tool to automatically convert addresses in @addr tags to uppercase #94

Open JohnP55 opened 3 months ago

JohnP55 commented 3 months ago

Ghidra shows addresses in lowercase, so copy-pasting from there requires manually changing the letters to uppercase. We could make a tool that automatically parses source files for addresses in @addr tags and converts them to uppercase, and add that tool to the CI workflow.

malleoz commented 3 months ago

I think this is an unnecessary amount of effort that would have to be allocated for a problem whose severity is minimal. After all, the aim is for contributions and PRs to eventually contain very few changes (including new functions) such that addr tags are added only a few times per PR. I think it is reasonable to not let our CI fail on capitalization issues, and instead require that PR review addresses any issue.

Alternatively, I actually think it may be fairly reasonable to create a PR for Ghidra itself that adds to the "Address Field" CodeBrowser options to enforce capitalization.

vabold commented 3 months ago

I think it is reasonable to not let our CI fail on capitalization issues, and instead require that PR review addresses any issue.

I disagree, I think the point of CI is to automate tasks that can be automated to detract the manual work in the review process. Requiring PR review potentially opens ourselves up to human error.

malleoz commented 3 months ago

I thought about it more, and basically the only argument I can sustain runs off the assumption that we don't 100% care about formatting consistency for function addresses. Regardless of how small the font for that may be currently (lol), I am starting to feel that we might as well automate it. If a new contributor's additions fail due to this requirement, it is very easy to communicate what the necessary change is, so this further supports that it should be fine to add this to CI.