mib added a comment. In D139945#4011466 <https://reviews.llvm.org/D139945#4011466>, @labath wrote:
> 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). @labath I'm taking note of your comment but as you said it's the pluggable design of scripted processes is not introduced in this patch. I also have some plans for Scripted Processes that may involve rethinking its pluggable design, so I'll keep your feedback in mind for when I implement those changes :) 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
