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

Fixes AnimationSet(std::string filename) uninitialized mFileName warning #965

Closed
wants to merge 2 commits into from

Conversation

cugone
Copy link
Contributor

@cugone cugone commented Aug 1, 2021

Closes #962.

Copy link
Collaborator

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

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

So this isn't too bad. It avoids changes to the public API would that introduce XML dependencies, so that's a huge plus.

On the downside, it is adding XML code back into the class, but only in a private manner, and only affecting the implementation file.

I feel like I might want to play with this a bit and see if we can get something better.


Related, the resource cache code relies on constructors. I had considered looking for ways to extend the resource cache code so it could alternatively use factory functions. Perhaps extend the template and give it a default factory function that delegates to the class constructor. I never put in the time to figure out the syntax though.

If we could update the resource cache code, there would be little reason to maintain a constructor that reads from a file. Particularly not if we want to remove dependencies on file parsing libraries. Instead, that can be moved to a factory function, such as already exists, and have the resource cache code use the factory function. That would enable us to lift the factory function out of the library into the executable project, along with dependencies on specific file processing libraries.

NAS2D/Resource/AnimationSet.h Outdated Show resolved Hide resolved
@cugone cugone requested a review from DanRStevens August 4, 2021 16:27
@DanRStevens
Copy link
Collaborator

I don't know if I'm quite ready to review this one yet. This one ties in with issue #972.

@DanRStevens
Copy link
Collaborator

Closing this to go with a hybrid solution in #1003.

Both solutions were similar, in that initialization was moved to the constructor body, rather than attempting to use the constructor initialization list. That was the key factoring in removing the warning. The other solution keeps most processing in functions in unnamed namespaces, whereas the solution here moved some XML code into the constructor, while leaving the corresponding function from the unnamed namespace intact but uncalled.

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.

AnimationSet C26495 warning
2 participants