labath added a comment. Looking this over again, I do see two open questions.
The first is that there is a disconnect between the place which *declares* something to be a plugin (the PLUGIN argument to add_lldb_library), and the place which *disables* something as a plugin (the macro invocation in the parent CMakeLists.txt). This creates a lot of potential for confusion and inconsistencies -- and indeed this patch already introduces some of those. The second is the boilerplate required to disable the relevant tests. Right now you've just added the couple of features that you needed to disable the plugins of your choosing, but if we did that for all plugins, the code to do that would become fairly repetitive. I am wondering if there isn't a way to solve both of these problems. For example, if we moved the disabling logic to the `add_lldb_library` function, then we could guarantee that only "real" plugins are eligible for being disabled. Also, since we already maintain a list of all plugin libraries, we could use this list to generate the appropriate lit features. This would mean that we would not be able to reuse the llvm logic for skipping a directory, but that is literally just 4 lines of code, so I don't think it would be much of an issue. That may even make sense, because that llvm machinery was meant for disabling tools or entire subprojects, and not as a fine-grained control mechanism like this. In fact, this feature is kind of more similar to one choosing which components go into libLLVM.so, which is handled by a separate mechanism (LLVM_DYLIB_COMPONENTS). Finally (though I don't know if you would consider this a feature or not), this way we might be able to arrange things so that the relevant plugins still get built even though they don't make their way into liblldb. That way one could still ensure that the relevant plugins build properly, and their unit tests could still run. ================ Comment at: lldb/source/Plugins/LanguageRuntime/CMakeLists.txt:1-3 +add_lldb_plugin_subdirectory(CPlusPlus) +add_lldb_plugin_subdirectory(ObjC) +add_lldb_plugin_subdirectory(RenderScript) ---------------- This isn't right. The actual plugins are located one level down (CPlusPlus/ItaniumABI, ObjC/AppleObjCRuntime, RenderScript/RenderScriptRuntime) ================ Comment at: lldb/source/Plugins/Process/CMakeLists.txt:1-19 if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android") - add_subdirectory(Linux) - add_subdirectory(POSIX) + add_lldb_plugin_subdirectory(Linux) + add_lldb_plugin_subdirectory(POSIX) elseif (CMAKE_SYSTEM_NAME MATCHES "FreeBSD") - add_subdirectory(FreeBSD) - add_subdirectory(POSIX) + add_lldb_plugin_subdirectory(FreeBSD) + add_lldb_plugin_subdirectory(POSIX) elseif (CMAKE_SYSTEM_NAME MATCHES "NetBSD") ---------------- Some of these (Utility, Linux, NetBSD) aren't "real" plugins. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73016/new/ https://reviews.llvm.org/D73016 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
