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

Clean up CLI singleton implementation #1209

Merged
merged 2 commits into from Jan 24, 2018
Merged

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Jan 21, 2018

A discussion I had on Friday made it clear that the CLI singleton implementation is not optimal or even particularly good. This minor refactoring obviates the need for the CLIDeleter class by putting the CLI singleton as a local static variable.

A discussion I had on Friday made it clear that the CLI singleton implementation
is not optimal or even particularly good.  This minor refactoring obviates the
need for the CLIDeleter class by putting the CLI singleton as a local static
variable.
@zoq
Copy link
Member

zoq commented Jan 22, 2018

Can you share some of the key points from the discussion?

@rcurtin
Copy link
Member Author

rcurtin commented Jan 23, 2018

Basically, using a raw pointer for a singleton makes it difficult to clean up---that's why the CLIDeleter class existed from before. If you can avoid explicitly allocating an object, then there's no need for CLIDeleter. So using something like a unique_ptr<> might be an option, but an inline static object is cleaner in my opinion (and the opinion of the other guy in the discussion). An inline static object is initialized at the time the method is first called, and destroyed during program termination (fortunately, because the Log objects and CLI objects are pretty decoupled, destruction order doesn't matter). So this allows us to remove the CLIDeleter object and even the NULL check in GetSingleton(), which is nice. It had been a long time since I had seen that, so I wish I had remembered earlier so I could have cleaned up the code years ago. :)

@zoq
Copy link
Member

zoq commented Jan 24, 2018

Thanks for the clarification, no more comments from my side.

@rcurtin
Copy link
Member Author

rcurtin commented Jan 24, 2018

Ok, if we both approve it I will merge it then.

@rcurtin rcurtin merged commit 582fec7 into mlpack:master Jan 24, 2018
@rcurtin rcurtin deleted the cli-refactor branch January 24, 2018 23:23
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