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

Tighten noexcept and virtuals on big five #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Tighten noexcept and virtuals on big five #70

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 9, 2016

As promised, here is a pull request with just the Big Five constructor changes.
Many of the constructors cannot throw, and are therefore marked noexcept.
A constructor for AudioSpec to allow interfacing with SDL2 more easily is added.
A few virtual destructors need clarification.

A few questions, however:

  1. AudioSpec has a default constructor that simply creates a NULL, or invalid, AudioSpec. As almost all of the classes here simply manage a pointer, could we not provide a default constructor that simply renders it NULL? It seems there are checks for NULL as is, so this would be a "drop in" addition, which will help with non-standard containers
  2. As many of these classes are by and large SDL pointer containers, it's a bit odd that some of them have virtual destructor and some do not. The general rule, I've found, is to have a virtual destructor on the classes that exist to be extended, but Font for example seems more like something that would be used with a "has a" relationship, not "is a".
  3. Is there any reason unique_ptr isn't being used to manage these raw pointers? It would make the owner more clear, and free up the classes to use/provide functionality, rather than manage. Generally, with the new "rule of zero", a class should either manage or use a resource, not both.

Any insight would be appreciated!

@ghost
Copy link
Author

ghost commented May 17, 2016

@AMDmi3 Is there any problem with this? It's been about a week, just wondering.

@AMDmi3
Copy link
Member

AMDmi3 commented May 18, 2016

Sorry, ENOTIME while I've decided to give it a thorough thought. For now, it still needs to be split into topic commits (and there are still some spaces, in AudioSpec.hh for example), preferably separate pull requests. noexcept may be merged right away, ctors probably fine too, devirtualizing destructors I don't really like as it kills extensibility. Not sure for all classes though, but I've had plans to make renderer completely virtual to provide several implementations. Maybe I should go with adaptor instead, and while it's cleaner from architecture and performance standpoints, it'll make changing render in runtime more complex and involve a lot of boilerplate code. And I also think there was a reason to keep defaulted ctors in source files instead of just headers.

AudioSpec has a default constructor that simply creates a NULL, or invalid, AudioSpec. As almost all of the classes here simply manage a pointer, could we not provide a default constructor that simply renders it NULL?

Gotta read the code to recall. I think there was some specific usecase with AudioSpec which required this, it is probably covered in tests or demos. As for other classes, the only allowed case for storing nullptr is a moved-from object, that's intentional policy.

The general rule, I've found, is to have a virtual destructor on the classes that exist to be extended, but Font for example seems more like something that would be used with a "has a" relationship, not "is a".

Also need to recall. Font rendering with SDL_ttf is too slow, so it needs extension very much, as in rendered string caches and dynamic glyph atlases. Doesn't seem like either would require Font class extensions, so you may be right.

Is there any reason unique_ptr isn't being used to manage these raw pointers? It would make the owner more clear, and free up the classes to use/provide functionality, rather than manage.

Sounds reasonable, gotta think that over.

@AMDmi3
Copy link
Member

AMDmi3 commented May 18, 2016

Gotta read the code to recall. I think there was some specific usecase with AudioSpec which required this, it is probably covered in tests or demos.

It's Wav class. It needs to store AudioSpec which is filled by SDL_LoadWAV which is called in Wav constructor body. By that time AudioSpec needs to be constructed in some way.

There are alternative solutions:

  • Use private default AudioSpec ctor + friend
  • Use parametric AudioSpec ctor with some random values
  • Store SDL_AudioSpec instead of AudioSpec in Wav (will need copy in Wav::GetSpec())

The default ctor for AudioSpec still looks like the most clean solution for me.

AMDmi3 added a commit that referenced this pull request May 18, 2016
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.

None yet

1 participant