[Lldb-commits] [PATCH] D73018: [lldb] Add SystemInitializerAllPlugins and delete copy-pasted Init code in SystemInitializerFull and SystemInitializerTest

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D73018#1830891 <https://reviews.llvm.org/D73018#1830891>, @teemperor wrote:

> > This still leaves the question of the script interpreter plugins, which are suspiciously *not* included in "all plugins". The script interpreters are quite special, so I think it's fine to handle them separately -- the question is just how to convey that distinction. Move them into a different top level folder? Call this `SystemInitializerMostPlugins` ?
>
> Yeah, those 'plugins' aren't actually standalone plugins but require the SWIG wrapper code to link (and the SWIG wrapper code is compiled in the API/ folder). We can fix this by moving the SWIG code into the AllPlugins folder (which would either be done in this commit or as a follow-up).


I'm afraid it's not that simple, as the swig code in turn depends on all the lldb public interfaces, which are provided by the API folder. So, even if you somehow manage to link this, it would still be nonsensical from the POV of lldb-test, as it does not include the lldb APIs. (That's why I said it may make sense to put these closer to/into the API folder.)

> 
> 
>> However, an even bigger question is what is the relationship of this patch and D73067 <https://reviews.llvm.org/D73067>? Right now, they seem to be taking the plugin system in two different directions, so I don't think it makes sense to accept either one before we decide what that direction is...
> 
> D73067 <https://reviews.llvm.org/D73067> is more focused on making the list of plugins to init/terminate depend on the CMake configuration, but even with D73067 <https://reviews.llvm.org/D73067> we still need to copy around the linking flags, include the def file and the other boilerplate. I don't think we should deduplicate identical code with macros (especially since the SystemInitializers already took the approach of making subclasses). So this patch solves the code duplication, D73067 <https://reviews.llvm.org/D73067> solves the fact that the plugin itself is not dependent on the actual plugin list.

Right, I now understand what you meant by the header file comment. Yes, these are somewhat independent subject, but they are still touching the exact same code (== rebase nightmare), and I don't even think they are fully orthogonal -- e.g. if we go through with the other patch, including the idea for generating the `#include` list, then the amount of boilerplate is reduced by like 90%. At that point, it's not clear to me whether introducing a separate "initializer" package is worth the final 10%. After that work is done, we may decide that we still want to do this, or we may see a better way to achieve the same goal (maybe something in cmake).

TTTT, though I agree that the current situation is bad, I also don't want to give too much emphasis on the "all plugins" case. I see being able to pick and choose what we include as a very nice property, and I'd rather encourage that instead. Selecting plugins at build time is one thing, but I'd like to also have it for different programs. We now just have three users (liblldb, lldb-server and lldb-test), but already each one requires different sets of plugins. If, in the future lldb-test continues to grow, then it may make sense to split it into multiple utilities (e.g. `lldb-objdump` instead of `lldb-test object-file`, where the former would only depend on the object file plugins). The main reason I haven't done anything about that yet is because in the current state of affairs, each tool would end up transitively depending on the whole world anyway...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73018/new/

https://reviews.llvm.org/D73018