Code review comment for lp:~ted/ubuntu-app-launch/registry-cleanup

Revision history for this message
James Henstridge (jamesh) wrote :

I don't really know this code base, but seeing a merge that converts a bunch of smart pointers into raw pointers (which is essentially what these reference variables are) raises some red flags.

Is there a guarantee that all instances of these classes will be destroyed before the associated Registry instance? If not, you're just replacing indeterminate cleanup of the registry object for dangling pointers.

Some alternatives that might do what you want include:

1. add a shutdown() method that will do the cleanup, and then in other classes check to see if the registry has been shut down before using it.

2. use std::weak_ptr, so these other classes won't keep the registry alive, but also won't leave a dangling reference.

It's a bit hard to say what would be appropriate without looking into it more.

review: Abstain

« Back to merge proposal