bulbazord planned changes to this revision. bulbazord added a comment. In D147009#4227163 <https://reviews.llvm.org/D147009#4227163>, @labath wrote:
> I already regret getting involved in this, but here it goes... Your effort and participation is greatly appreciated. I'm sorry if this is frustrating. > Do you actually expect we will have multiple ObjectFile subclasses > implementing the `ObjectFileCreateInstanceWithDelegate` interface? Because if > not, then I think this is just a very roundabout way of being able to satisfy > the syntactic requirements for calling ObjectFileJIT a plugin. That's exactly what this is. If we had "ScriptedObjectFiles" or something similar to this (like with what @mib is doing with ScriptedProcess) then this abstraction might make more sense but I'm not sure if that will be a thing. > However, semantically, I don't think ObjectFileJIT is really a plugin, > because a fundamental property of a plug**in** is that one can plug it > **out** (and still have the core functionality working). If I compiled LLDB > without ObjectFileELF, then it would work just fine (sans the capability to > open ELF files). If I plug ObjectFileJIT out, then **all** expression > evaluation (a core functionality) becomes broken, regardless of which target > I'm debugging (or anything else). > > I think that LLDB (falsely) equates the concept of a "plugin" with that of a > "subclass". `ObjectFile` is clearly a class that was meant to be pluginized > by subclassing. However, that doesn't mean that **every** subclass of > `ObjectFile` needs to be a plugin. > > I find it very unlikely that someone is going to implement a different > ObjectFile plugin that would take an ObjectFileDelegate and expose it as an > ObjectFile instance. The idea just doesn't make sense. I wasn't around when > this got created, but I'm pretty sure that's not what the author had in mind. > > I think that the situation here is reversed. ObjectFileJIT is **not** the > plugin. ObjectFile**Delegate** is. And ObjectFileJIT is just an interface > adaptor to present the delegate as an object file. To me, this puts it into > the same category as e.g. ProcessTrace, which is a non-plugin subclass of a > pluggable base, which implements a generic tracing capability on top of a > trace **plugin**. > > Now, with all of this in mind, I'd like to propose a simpler solution to the > plugin dependency problem: just drop the plugin pretense and make > ObjectFileJIT a core class. We can move it to a core folder (next to the > ObjectFile class -- like ProcessTrace), to satisfy the formal requirements. > And, we probably don't need to do anything else... I'm alright with this solution. We also don't need to rename "ObjectFileJITDelegate" to just "ObjectFileDelegate" so it'll be very clear what it is. I'm honestly not entirely sure why ObjectFileJIT is a plugin to begin with, but the argument you've laid out is very compelling for why it shouldn't be. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147009/new/ https://reviews.llvm.org/D147009 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
