Skip to content

Add constructors and operators #142

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

Merged
merged 11 commits into from
Sep 11, 2021

Conversation

maciejewiczow
Copy link
Contributor

@maciejewiczow maciejewiczow commented Sep 10, 2021

Added constructors and assigmnent operators according to the rule of five to all other classes that needed it (according to the appropriate Unload*() functions found in raylib).

Also some cosmetic fixes:

  • Replaced all uses of NULL with nullptr
  • Fixed a memory leak in ModelAnimation::Load method

@RobLoach
Copy link
Owner

This is absolutely fantastic! Thanks so much for pushing it forwards. Is there anything else you think we should get in there before merging?

@maciejewiczow
Copy link
Contributor Author

Idk, nothing really comes to my mind right now.
There is one thing about the const modifier in various class methods. There are more places that it could be added in, but then these methods would need to be duplicated, for const and non-const version.
Without the duplication const and non-const methods could not be mixed in chain calls. But the duplication process can be quite laborious and I did not include it here.

@maciejewiczow
Copy link
Contributor Author

Also as a general note, not necessarily related to this PR, it would be neat if all the methods that take std::string would also provide a const char* alternative, as the string class always performs a copy when you pass in a const char*, even when the argument is declared as a const reference. So the char pointer should be preferred unless you really need some of the string class functionality, because conversion from char* to string is more taxing than from string to char*.

@maciejewiczow
Copy link
Contributor Author

Also have you maybe considered using some framework for the tests?

@RobLoach
Copy link
Owner

RobLoach commented Sep 10, 2021

So the char pointer should be preferred unless you really need some of the string class functionality, because conversion from char* to string is more taxing than from string to char*.

Having const char* overrides seems like a good idea. I've heard some things on string_view. Would that be worth considering?

Also have you maybe considered using some framework for the tests?

Catch2 use to be in there, but took it out because it was causing issue with some compilers.

@maciejewiczow
Copy link
Contributor Author

Having const char* overrides seems like a good idea. I've heard some things on string_view. Would that be worth considering?

I don't think string view would be a good match here. String view is good for efficiently referencing parts of other, existing string. I think plain old const char* will be good fit here.

@RobLoach RobLoach merged commit 3d4399e into RobLoach:sound-constr Sep 11, 2021
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.

3 participants