labath added a comment. In D63363#1549500 <https://reviews.llvm.org/D63363#1549500>, @jingham wrote:
> In D63363#1545427 <https://reviews.llvm.org/D63363#1545427>, @labath wrote: > > > Although this is technically correct and pretty consistent with our other > > "plugins", I can't help but feel that it's incredibly wasteful. Each of the > > XXXSignals.cpp files is less than a 100 lines long (with the licence header > > and all bolierplate) and it's unlikely to ever grow beyond that. And > > essentially, all these files do is define a single enum. The reason they > > are this long is because the UnixSignals class is already over-engineered > > (e.g. I don't see why LinuxSignals needs to be a separate class, or why it > > needs to repeat the descriptions/default stop values for each of the > > signals). Making this a plugin would just add another chunk of boilerplate > > on top of that. > > > > I don't know about others, but I'd rather us move in a direction which > > reduces the amount of boilerplate instead of adding more of it. In my ideal > > world, each of these signal definitions would just be a bunch of (number, > > name) pairs. This doesn't have/need to be a class or a plugin; a single > > constexpr variable would suffice for that. Then we'd just cross-reference > > this mapping with another one which gives the default stop values and > > descriptions for each of the signals, and that's it. > > > > I know I am repeating myself, but each time I say this, it's because I find > > another reason for it: I think we should start a new library which I would > > roughly define as "utilities for describing and manipulating various > > low-level aspects of processes, but which is agnostic of any actual process > > class". The idea would be that we can shove here all classes which are > > shared between lldb-server liblldb. UnixSignals would be one candidate for > > it. AuxVector, MemoryRegionInfo are others. `Plugins/Process/Utility` > > (where most of the signal classes live) would be a pretty good place for it > > already, were it not for the "Plugins" part (it would be weird for > > non-plugin code to depend on something called a "plugin"). However, maybe > > we could just create a new top-level library called "ProcessUtil" (or > > whatever name we come up with) and move the relevant stuff there. > > > > Anyway, TL;DR: I think this should be handled differently. However, if > > others are fine with this approach, then feel free to ignore me. > > > How would you bind a particular variant of UnixSignals to the process plugin > that it goes along with in this scenario? The same way as it is done now -- you pass in an ArchSpec, and get back a list of signals in return. The only difference would be that the implementation of "finding the right set of signals for a given ArchSpec" wouldn't be distributed over 5 subfolders, but it would happen in a single place. (I think that this code is so simple that we should shove all of it into a single file -- that would make it easier to share common stuff like signal descriptions between implementations) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63363/new/ https://reviews.llvm.org/D63363 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
