JDevlieghere wrote:

Based on the title of this PR I was expecting something slightly different, so 
maybe I'm missing something. But if we want to make this an LLDB plugin (which 
I agree we should ), I would expect the `PluginManager` to manage the instance 
rather than the plugin doing itself. With the current patch, I don't see how 
you're taking advantage of it being a plugin.  

In the `PluginInterface`, I would expect a `Create` instance, something like 
this:

```
class TelemetryManager : public PluginInterface {
public:
  static lldb::TelemetryManagerUP Create(const llvm::telemetry::Config& config);

  TelemetryManager() = default;

  virtual <methods you need for TelemetryManager> = 0;
};
```

And then the `PluginManager` manages the plugin's lifetime:

```
typedef PluginInstance<TelemetryManagerCreateInstance> TelemetryManagerInstance;
typedef PluginInstances<TelemetryManager> TelemetryManagerInstances; 

static TelemetryManager &GetTelemetryManagerInstance() {
  static TelemetryManager g_instances;
  return g_instances;
}

bool PluginManager::RegisterPlugin(
    llvm::StringRef name, llvm::StringRef description,
    TelemetryManagerCreateInstance create_callback, const 
llvm::telemetry::Config& config) {
  return GetTelemetryManagerInstances().RegisterPlugin(name, description, 
create_callback,
                                             config);
}

bool PluginManager::UnregisterPlugin(
    TelemetryManagerCreateInstance create_callback) {
  return GetTelemetryManagerInstances().UnregisterPlugin(create_callback);
}

TelemetryManagerCreateInstance
PluginManager::GetTelemetryManagerCreateCallbackAtIndex(uint32_t idx) {
  return GetTelemetryManagerInstances().GetCallbackAtIndex(idx);
}
```

And then `Create` would iterate over the plugins:

```
lldb::TelemetryManagerUP TelemetryManager::Create() {
  uint32_t idx = 0;

  while (TelemetryManagerCreateInstance create_instance =
             PluginManager::GetTelemetryManagerCreateCallbackAtIndex(idx++)) {
    if (auto device_up = (*create_instance)())
      return device_up;
  }
  return {};
}
```

And then `GetInstance` would look like this:

```
TelemetryManager *TelemetryManager::GetInstance() {
  if (!m_instance)
    m_instance = TelemetryManager::Create();
  return m_instance.get();
}
```


https://github.com/llvm/llvm-project/pull/126588
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to