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

game: fix order of init/deinit in ServiceManager #12431

Closed
wants to merge 1 commit into from

Conversation

FernetMenta
Copy link
Contributor

see title

@garbear /cc

@FernetMenta
Copy link
Contributor Author

jenkins build this please

@FernetMenta FernetMenta requested a review from garbear July 6, 2017 14:22
@garbear
Copy link
Member

garbear commented Jul 6, 2017

What ever happened to the LIFO idea? This is going to keep biting us without a LIFO.

@garbear
Copy link
Member

garbear commented Jul 6, 2017

CInputManager is accessed from main() on linux. It doesn't need to be initialized, it just has to be created. CAppParamParser sets some properties that are read upon its initialization.

@FernetMenta
Copy link
Contributor Author

this pr makes it lifo

in:

m_announcementManager
m_gameServices.reset(new GAME::CGameServices);
m_peripherals.reset(new PERIPHERALS::CPeripherals);
m_inputManager.reset(new CInputManager);

out:
m_inputManager.reset();
m_peripherals.reset();
m_gameServices.reset();
m_announcementManager.reset();

prior to this it was no lifo because announcementManager is supposed to be first. your ctor init made game first.

@FernetMenta
Copy link
Contributor Author

CInputManager is accessed from main() on linux

in this case it shouldn't be owned by ServiceManager

@FernetMenta
Copy link
Contributor Author

btw: kodi-test crashed because peripherial depends on announcementManager

@garbear
Copy link
Member

garbear commented Jul 6, 2017

in this case it shouldn't be owned by ServiceManager

In the case where it is owned by ServiceManager, main() shouldn't call into CInputManager.

The change here is correct. I can refactor the CAppParamParser issue later today.

@garbear
Copy link
Member

garbear commented Jul 6, 2017

BTW #12426 can be reverted once we take care of this mess.

@FernetMenta
Copy link
Contributor Author

@garbear do you want to take over this pr?

@garbear
Copy link
Member

garbear commented Jul 6, 2017

No :) But I think I see how things should be so I'll tackle this issue and open a new PR.

@FernetMenta
Copy link
Contributor Author

FernetMenta commented Jul 6, 2017

that's what I meant. if you open a new PR, I can close this one, right?

@garbear
Copy link
Member

garbear commented Jul 6, 2017

Superseded by #12432

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

3 participants