Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sh3_arc* cleanup #109

Merged
merged 7 commits into from
Sep 4, 2017
Merged

sh3_arc* cleanup #109

merged 7 commits into from
Sep 4, 2017

Conversation

z33ky
Copy link
Collaborator

@z33ky z33ky commented Aug 19, 2017

I decided the first step towards #95 is some cleanup in the arc* structs.
The rest should follow in the next days so I'd suggest you leave this PR unmerged until they are in. But you can start reviewing each commit (I'm gonna change each struct in a separate commit), hence me opening the PR already.

@z33ky z33ky requested a review from Quaker762 August 19, 2017 20:24
@z33ky z33ky force-pushed the resource-loading branch 2 times, most recently from 4999340 to cad07df Compare August 19, 2017 20:53
*/

/** Type check header */
struct Header
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I start using Uppercase for the first letter for structs from now on??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh that's just my personal style bleeding through. I'll fix it.

@Quaker762
Copy link
Member

@z33ky Have you made the changes to this?? If so, I'll merge it :)

@z33ky
Copy link
Collaborator Author

z33ky commented Sep 1, 2017

Sorry, I've been distracted last weekend by a SBC I got and have been busy otherwise this week.
I should be able to push two more commits today though; the changes are in, I just need to split them up a bit. After this there's only vfile left.

@z33ky
Copy link
Collaborator Author

z33ky commented Sep 1, 2017

There we go. I'll squash the fixup! after review and now only sh3_arc_vfile is missing the refactorization treatment.

@z33ky z33ky force-pushed the resource-loading branch 2 times, most recently from 9d9d49f to ea10ac2 Compare September 1, 2017 09:56
@Quaker762
Copy link
Member

Quaker762 commented Sep 3, 2017

Looks really good, much cleaner than what we originally had. I'm curious though, how does this line std::string &&subarcName work, with the double ampersand??

Apart from that, squash when you're ready :)

@z33ky
Copy link
Collaborator Author

z33ky commented Sep 3, 2017

It's an rvalue-reference. Basically it's a reference where the caller promises that the value will not get used any further, which means you can "move" stuff out of it.
So for the case of the std::string, the copy-constructor (assuming no CoW (Copy-on-Write) semantics) needs to allocate new memory and copy the characters over, while the move-constructor just takes the existing memory from the other string and changes that string to be empty, so its destruction will not delete the memory.

There's also some minor additional refactoring, but generally this just
moves some code around.
Also applies some minor phrasing changes.
This also shuffles the subarc file-loading functionality around, so
some code moves from (formerly) sh3_arc to sh3::arc::subarc.
@z33ky
Copy link
Collaborator Author

z33ky commented Sep 3, 2017

Rebased onto master and squashed the fixup. There are two new commits on top for vfile with which I now consider this PR mergeable after you approve.

@Quaker762 Quaker762 merged commit 8cdab77 into Palm-Studios:master Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants