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

Change ptrs fix segfault #81

Closed
wants to merge 2 commits into from
Closed

Change ptrs fix segfault #81

wants to merge 2 commits into from

Conversation

DoctorTeeth
Copy link
Contributor

This does the following 2 things.

  1. Replace auto_ptr with unique_ptr.
    I think it's a good idea to do this now because:
    First: auto_ptr has been deprecated, and will actually be deleted in the future, according to Herb Sutter:
    http://herbsutter.com/2014/11/24/updates-to-my-trip-report/
    Second: right now it's still a drop-in replacement. unique_ptr has move-only semantics, but you're not doing any copying of auto_ptrs right now. In the future, if you try to do copying of ptrs, the compiler will stop you. If you then still want to do copying, you can switch to shared_ptr.
  2. Instead of segfaulting if someone tries to e.g. get the list of legal actions before a ROM has been loaded, just exit with a message. I think that this is a good change because
    First: segfaulting is always bad
    Second: this change would have saved me 5-10 minutes of debugging time, so I expect a similar thing might be true for other people.

…as not. Because none of the auto_ptrs were being copied, this replacement was drop-in.
@mgbellemare
Copy link
Contributor

Thanks for the fix! I'd like to keep the code C++11 free for just a little bit longer to provide support for certain compilers (MSVC comes to mind). We had an earlier discussion on the topic, however, so unique_ptr will certainly make its appearance in ALE 6-7 months from now.

I'd change the calls to printf to std::cerr (and actually to the logging method that @alcinos is working on). Rather than calling exit(), I would throw an exception, since exit() may not call the appropriate destructors. This also lets the caller recover (somehow).

Since most of your commits are about unique_ptr, I can either do the proposed changes myself & discard this PR, or let you take care of it?

@DoctorTeeth
Copy link
Contributor Author

Very generous of you to support MSVC ;)

I just made a different PR that throws exceptions. It's not clear you need a separate log message in that case, because the user will see the exception.

@mgbellemare
Copy link
Contributor

Agreed. Thanks.

@mgbellemare mgbellemare mentioned this pull request Sep 13, 2015
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

2 participants