dmpots 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.
More of a theoretical concern. I have a later patch that adds a `bool enabled`
member to the PluginInstance. so updates to that would be ignored. But we could
store the enabled state outside the PluginInstance by changing the vector to a
pair of `{instance, enabled}`.
There were lots of plugins and I did not look in detail to see if they had
internal state. But as you said if we remove the const overload and everything
still works then we are a probably safe with the current set of instances.
As long as we are ok with a change from reference semantics to copy semantics
for future use cases as well then I think we could go this route.
> > 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.
In this sense returning a copy is safer (but we would need to protect access to
the underlying vector while making the copy if we really think its a problem).
> > 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.
If you are ok with removing the reference semantics then we should probably
just return a copy since that is the simplest solution.
https://github.com/llvm/llvm-project/pull/132884
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
