Refactor AnimationSet loading

[cugone]

Closes #962

Delegating to a copy constructor is potentially an unsafe operation as well as MSVC complaining that member variables were not initialized. All member variables are now initialized in the header by default.

I took the opportunity to refactor the loading code because of the MSVC warning. The various process functions were private to the implementation file via an unnamed namespace as well as only used in one place. Therefore, they made great candidates for Immediately Invoked Initializing Lambdas which aren't accessible outside the function they are declared, reducing their scope even further.

[cugone]

@ldicker83 or @DanRStevens The issues Codacy complains about can be safely ignored. I do not have the proper permissions to ignore them:

---

Codacy detected an issue:

Message: Function parameter 'fileName' should be passed by const reference.

Occurred on:

• Commit: 0c2ef49
• File: NAS2D/Resource/AnimationSet.cpp
• LineNum: 53
• Code: AnimationSet::AnimationSet(std::string fileName) :

Currently on:

• Commit: 0c2ef49
• File: NAS2D/Resource/AnimationSet.cpp
• LineNum: 53

---

Codacy detected an issue:

Message: Check buffer boundaries if used in a loop including recursive loops (CWE-120, CWE-20).

Occurred on:

• Commit: 0c2ef49
• File: NAS2D/Resource/AnimationSet.cpp
• LineNum: 61
• Code: xmlDoc.parse(filesystem.read(mFileName).c_str());

Currently on:

• Commit: 0c2ef49
• File: NAS2D/Resource/AnimationSet.cpp
• LineNum: 61

[DanRStevens]

Could you provide details or a reference to explain the following:

Delegating to a copy constructor is potentially an unsafe operation

It might be the MSVC warning is simply a bad warning. It wouldn't be the only case where MSVC warns about things it probably shouldn't. In particular, the use of regular enum value names from C imports, such as SDL2, results in a warning that really shouldn't be generated.

---

This change kind of goes against refactoring to remove direct dependence on XML. Here that's been added back by putting an XML include in the AnimationSet header file, and incorporating XML types into member methods. Using methods from an unnamed namespace avoids this direct dependence.

---

I find named methods in an unnamed namespace to be more clear than unnamed immediately invoked function expressions. If you were to take scope limitation to the extreme, you might find inlining the code to be a natural conclusion, though that wouldn't necessarily be a great organization that would aid readability.

---

I would like to solve the MSVC warning mentioned above, though I feel this change takes us farther from where we want to be. I would like to back out of this change set.

[DanRStevens]

I think the more proper solution here is to use std::move.

mFileName{std::move(fileName)}

You don't want to be passing by const& when using std::move. That would end up silently forcing a copy instead.

[cugone]

Could you provide details or a reference to explain the following:

Delegating to a copy constructor is potentially an unsafe operation

Any and all documentation, websites, tutorials, etc, that Google-Fu brings up about delegating to the copy-constructor never gives an example of using the copy-constructor as the delegating constructor. Nor is it mentioned as a valid or even invalid use.

[cugone]

I've created a separate PR just addressing the uninitialized variable issue. See #965

[DanRStevens]

Any and all documentation, websites, tutorials, etc, that Google-Fu brings up about delegating to the copy-constructor never gives an example of using the copy-constructor as the delegating constructor. Nor is it mentioned as a valid or even invalid use.

... so basically what you're saying is there's no documentation to state that it's invalid. 😉

I'm pretty sure the warning is wrong. Nevertheless, it would be nice to have code that doesn't generate warnings.