labath added a comment. In D139945#4009427 <https://reviews.llvm.org/D139945#4009427>, @JDevlieghere wrote:
> In D139945#3999351 <https://reviews.llvm.org/D139945#3999351>, @labath wrote: > >> For a "plugin", the scripted process is sure getting a lot of special >> handling in generic code. (I know this isn't being introduced here, but I >> wasn't involved in the previous review -- and I'm not actually sure I want >> to be involved here). I don't think that's necessarily a bad thing in this >> case, but maybe we should not be calling it a plugin in that case? We >> already have a couple of precedents for putting implementations of >> "pluggable" classes into generic code -- ProcessTrace for instance. And just >> like in the case of ProcessTrace (where the real plugin is the thing which >> handles the trace file format), here the real plugin would the the scripting >> language backing the scripted process? >> >> (Apart from that, this patch seems fine.) > > Maybe one way around this is to have some generic metadata that gets passed > to the plugin, that can be different per plugin? That might work, but I don't think it's really necessary. My only issue is one of nomenclature. I don't see a problem with having a concrete Process subclass in the lldb core. My problem is with calling that class a "plugin", and pretending it's pluggable, but then introducing backdoor dependencies to it (usually by referring to with via its (string) plugin name). You could try to solve that by making it genuinely pluggable, but that could be overkill, given that it already contains *is* pluggable, only in a different way -- one can plugin a different scripting language to implement the bindings (and then obviously the user can plug in to implement those bindings). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139945/new/ https://reviews.llvm.org/D139945 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
