[PATCH] D51748: cmake: Refactor add_llvm_loadable_module()

[Tom Stellard via Phabricator via llvm-commits]

tstellar added inline comments.


================
Comment at: cmake/modules/AddLLVM.cmake:677
+  add_llvm_library(${name} MODULE ${ARGN})
   if(NOT TARGET ${name})
     # Add empty "phony" target
----------------
philip.pfaffe wrote:
> beanz wrote:
> > I don't believe there is any situation where calling add_llvm_library with `MODULE` would ever result in not creating the target, so this condition is probably not needed, which makes me think we should probably just get rid of `add_llvm_loadable_module` entirely.
> Looks like it won't be created if(NOT LLVM_ENABLE_PLUGINS AND NOT (ARG_PLUGIN_TOOL AND LLVM_EXPORT_SYMBOLS_FOR_PLUGINS))
What about when LLVM_ENABLE_PLUGINS is OFF? I tried a similar solution for D50668, and I thought this was why the Windows bots failed.

The other thing add_llvm_loadable_module does is:

set_target_properties(${name} PROPERTIES FOLDER "Loadable modules")

Would this be OK to drop?


Repository:
  rL LLVM

https://reviews.llvm.org/D51748