[Lldb-commits] [lldb] [lldb] Remove raw access to PluginInstances vector (PR #132884)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

> We did consider this approach. There are some drawbacks beyond the performance issue, but the tradeoff may be worth it.
> 
> If we define the method like: `std::vector<PluginInstance> GetEnabledInstances()` then we are returning copies of the PluginInstance. Anything that modifies the internal state of the `PluginInstance` will be lost since it is operating on a copy (e.g. `GetEnabledInstances().front().DoSomethingToModifyInternalState()`).

Is that a theoretical concern or actually something that happens (or will happen with your patches)? Looking at the existing code, it doesn't look like anyone is modifying the instance and I can remove the non-const overload and we still compile. If anything, we probably don't want anyone to change these things as we're iterating over them.

> We could also return pointers to the plugin instances like`std::vector<PluginInstance*> GetEnabledInstances()`. But these would be pointers to the locations in the vector stored in the `PluginInstance` class. So if a new plugin is added to the PluginInstances list the pointers could be invalidated. Probably not an issue with the current code, but could be unsafe to hand out those references in general.

While definitely unsafe an something I think we shouldn't do, that's a problem today too if someone were to modify the instance list from another thread or take the address of one of its elements. 

> I suppose another way would be to keep a `std::vector<shared_ptr<PluginInstance>>` and then creating a copy would not be an issue since any updates would also modify the original object.

I'd like to avoid shared_pointers if possible, but we could achieve the same thing by storing unique pointers in the vector and handing out raw points like you suggested above. 

https://github.com/llvm/llvm-project/pull/132884