[PATCH] D51748: cmake: Refactor add_llvm_loadable_module()

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: cmake/modules/AddLLVM.cmake:677
+  add_llvm_library(${name} MODULE ${ARGN})
   if(NOT TARGET ${name})
     # Add empty "phony" target
----------------
tstellar wrote:
> 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?
@philip.pffae, you're right. I think if the goal is to make `add_llvm_library` support `MODULE` I still think we should have it replace `add_llvm_loadable_module`. Much of what is in the later is already implemented in `add_llvm_library` anyways (like all the install goop).

It might make sense to handle the case where the target isn't created in `add_llvm_library` with an early return after the `llvm_add_library` call that just creates the dummy target.

@tstellar, It is probably best to just move the folder setting into `add_llvm_library` if `MODULE` is set. That is really just for sorting things in the IDE generators, so it probably makes sense.


Repository:
  rL LLVM

https://reviews.llvm.org/D51748