https://github.com/labath commented:
This is definitely better than what you had before, but I still think it's more complicated than it needs to be. For one, I'd like to understand why is there a need for separate `TelemetryManager` and `TelemetryConfig` fields. If the downstream implementation is going to be in charge of creating the telemetry manager, why does it need to bother with calling `SetTelemetryConfig`? A good way to demonstrate how this is supposed to be used in practice (and also provide some test coverage) would be to write a unit test which creates a simple telemetry vendor plugin and goes through the motions of creating and registering it. It doesn't have to be big -- I'm only interested in the mechanics of registration here, not data collection -- but it should show how one goes about to create and then access the telemetry manager. If we can reduce the size this class (which I'm 80% certain we can), then I am also thinking that maybe it doesn't need to exist at all, as the remaining (static) functions could be moved into the TelemetryManager class (so that TelemetryManager (not vendor) is *the* plugin). I think that would also be more consistent with the lldb plugin concept as e.g. ObjectFile instances are created and managed by ObjectFile subclasses -- there isn't a special ObjectFileVendor hierarchy to do that (the SymbolVendor, which you may be modelling this after, is a bit of an exception here, but I don't want to use it as a model precisely because it's so simple that it might not need to exist). 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
