labath added a comment. Overall, I like this, but I have a lot of comments:
- I actually very much like the plugin namespace idea -- it makes it harder to use plugin code from non-plugin code, and allows plugins of the same category (e.g. ProcessLinux and ProcessNetBSD) to define similar concepts without fear of name clashes. I do see how that gets in the way of auto-generation though, so putting the initialization endpoints into a more predictable place seems like a good compromise. I wouldn't put the entire main class into the lldb_private namespace though, as that is also something that should not be accessed by generic code. Ideally, I'd just take the `Initialize` and `Terminate` (TBH, I don't think we really need the terminate function, but that may be more refactoring than you're ready for right now), and put it into a completely separate, predictibly-named file (`void lldb_private::InitializeProcessLinux()` in `Plugins/Process/Linux/Initialization.h` ?). That way the "main" file could be free to include anything it wants, without fear of polluting the namespace of anything, and we could get rid of the ScriptInterpreterPythonImpl thingy. - it would be nice to auto-generate the `#include` directives too. Including everything and then not using it sort of works, but it does not feel right. It should be fairly easy to generate an additional .def file with just the includes... - The way you disable ScriptInterpreterPython/Lua plugins is pretty hacky. Maybe the .def file should offer a way to exclude entire plugin classes? #ifdef LLDB_WANT_SCRIPT_INTERPRETERS ScriptInterpreterPython::Initialize(); // or whatever #endif - some of the things you initialize are definitely not plugins (e.g. `Plugins/Process/Utilty`, `Plugins/Language/ClangCommon`). I think that things which don't need to register their entry points anywhere should not need to have the Initialize/Terminate goo... Can we avoid that, perhaps by making the "plugin" special at the cmake level. Since this code is used only by other plugins, it makes sense to have it somewhere under `Plugins/`, but it is not really a plugin, it does not need to register anything, and we do not need to be able to explicitly disable it (as it will get enabled/disabled automatically when used by the other plugins). - Having this as one bit patch is fine for now, but once we agree on the general idea, it should be split off into smaller patches, as some of the changes are not completely trivial (like the bunching of the macos platform plugins for instance) In D73067#1830408 <https://reviews.llvm.org/D73067#1830408>, @JDevlieghere wrote: > In D73067#1830304 <https://reviews.llvm.org/D73067#1830304>, @compnerd wrote: > > > Do we need to worry about ordering of the plugins? > > > Yes, it appears to matter... I'm still seeing test failures which I need to > debug. I'm not sure yet if they're because of missing initializers (that > don't have a corresponding CMake plugin) or because of the ordering. Order of plugins within a kind should not matter, but it's possible it does. Some of the entry points may need to be changed so that they are less grabby. I'm not sure if order of plugin kinds matters right now, but I don't think it would be wrong if it did (e.g. have symbol files depend on object files). However, it looks like this approach already supports that... Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73067/new/ https://reviews.llvm.org/D73067 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
