OdysseyDecomp
OdysseyDecomp copied to clipboard
MonsterDruide1
Workflow: simplify `progress.yml` and remove unusable `inline` functions from headers repo
This PR simplifies the progress worklow by removing setting up and building the project from it. I also made it so that copy-headers.sh removes lines with inline functions from the al and game folders that don't appear in the executable since calling them would cause a crash
I have tested the inline function remover inside a local copy of the headers repo but not the progress workflow
This change is 
I disagree. For functions more than like two lines it wouldn't make much sense that the devs made a cpp only function for something, it also wouldn't make sense for them to mark a random member function as inline, which is why I think they probably just used the same duplicate code in multiple functions. So it was propably a part of the member functions we're inling it into. Now since this decomp is about high quality code, it's inlined and I specifically moved it to be a member function because it made the code way cleaner than using a manual this pointer and you aproved that (talking about the AnagramAlphabetCharacter hack functions here)
If the functions only used mCapTargetParts then I would be fine with making it a non-member function that only takes that in as a param, but passing in the this pointer of a class to a function that’s only meant for that class just doesn’t feel right imo
Also, I think it would be good that the headers repo wouldn’t have function declarations for functions that don’t have a symbol since calling those would also cause a crash, but that would be a bit hard to implement.
When everything works out, the symbol-less functions wouldn't even be listed in the header in the first place, so they also wouldn't show up in the Headers repo. We currently can't check for that yet, but hopefully soon - with the linker checks.
Regarding the AnagramAlphabetCharacter functions specifically: Do you know if inline-marked member functions still show up in the executable?
Looking at #361, which has some better examples of useful inline member functions, I'm reconsidering this - these functions are used often enough to warrant existing on their own, and use member variables within the function itself that are not loaded at the start.
Give me a bit more time to think about this. To avoid that I forget about this, please write a comment, so I'll get a notification in my "queue".
As discussed a while ago, unless something very specific happens (which I don’t remember what, but it has never been an issue) inline functions don’t appear in the executable regardless of if they’re member functions or cpp only. When I add the mismatch to the linter soon, I’ll also add a check that fails on non inline function definitions that do not match anything in the csv
Yep exactly. It’s an exception that doesn’t operate on lines with “= default” since those ones can always be generated from headers without the cpp files and won’t cause crashes. “!d” just deletes all lines that match
As for the multiline thing: Currently multi line inline functions in headers in this project are all ones that have their body implemented in the header and don’t need to be removed. This is why I specifically made it so that it doesn’t remove the first line of those by checking for the semicolon at the end of the function declaration. Since inline member functions have the this pointer available, they usually don’t take too many params and we can name them how we want so the names usually aren’t that long. So I don’t see this as an issue and if there somehow is a inline member function with a multiline declaration, it just won’t get changed in any way so everything including it from the headers repo would still compile
And more about the sed syntax: I don’t remember exactly why I chose specific things but I do remember that it found all the edge cases I didn’t want it to change and removed all the cases I wanted it to remove in the current repo at the time of opening this PR
If you're looking into introducing a new solution for a problem, I think it should also catch all edge cases, in this case including multi-line function declarations - otherwise it just screams for problems at some point later, when we don't expect it.
Ig that's fair. I'll try to figure something out with multiline mode later
Oh, I accidentally pushed an identical copy of master, that's why it auto closed lol
But anyway, I've moved the inline removal logic (as well as the access modifier replacement thing) into it's own python script as suggested in DMs which now adds multi line support, so this PR should be ready for rereview