[PATCH] D28869: [CMake] Fix `is_llvm_target_library` and support out-of-order components

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: cmake/modules/LLVM-Config.cmake:12
 
 function(is_llvm_target_library library return_var)
+  cmake_parse_arguments(ARG "ALL_TARGETS;INCLUDED_TARGETS;OMITTED_TARGETS" "" "" ${ARGN})
----------------
bryant wrote:
> Could there be some documentation for this function? Something like:
> 
> ```
> # Checks if ${library} is a member component of some target in a list of
> # targets. This target list is one of:
> #     ALL_TARGETS (the default, if no target list specifier is given);
> #     INCLUDED_TARGETS;
> #     OMITTED_TARGETS.
> ```
> 
> Feel free to word-smith that.
Will do.


================
Comment at: cmake/modules/LLVM-Config.cmake:121
 
+  get_property(LLVM_TARGET_REGISTRATION_COMPLETE GLOBAL PROPERTY LLVM_TARGET_REGISTRATION_COMPLETE)
+
----------------
bryant wrote:
> Just a small nit, not mission-critical: Could `LLVM_TARGET_REGISTRATION_COMPLETE` (both the local var and global prop) be shortened to, say, `LLVM_TARGETS_REGISTERED`, `LLVM_SCANNED_TARGETS`, `LLVM_GOT_ALL_TARGETS`? It's kind of a finger-ful to type.
`LLVM_HAZ_ALL_TARGETS` has a nice ring to it :-).

You're right that is a finger-full. I'll shorten it.


================
Comment at: cmake/modules/LLVM-Config.cmake:128
+  foreach(c ${link_components})
+    is_llvm_target_specifier(${c} iltl_result ALL_TARGETS)
+    if(iltl_result AND NOT LLVM_TARGET_REGISTRATION_COMPLETE)
----------------
bryant wrote:
> If we know that all targets have been scanned, can we bypass this check entirely?
> 
> ```
> if(NOT LLVM_TARGET_REGIS)  # sorry, that name is just too long
>   foreach(c ${link_components})
>     is_llvm_target_specifier(..)
>       if(iltl_result)
>         message(FATAL_ERROR ...)
>       endif()
>     endforeach()
> endif(NOT LLVM_TARGET_REG)
> ```
Yes, that is a better expression of the logic.


================
Comment at: cmake/modules/LLVM-Config.cmake:242
+          # A missing library to a directly referenced omitted target would be bad.
           message(FATAL_ERROR "Library `${c}' not found in list of llvm libraries.")
+        else()
----------------
bryant wrote:
> If we arrive here, it's precisely because one of the dependencies is a sub-component of an omitted target, right? Can you make the error message more specific to reflect this?
Will do.


================
Comment at: cmake/modules/LLVM-Config.cmake:248
+          # that this is valid here.
+          list(APPEND expanded_components LLVM${c})
         endif()
----------------
bryant wrote:
> `list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>")` ? Then we won't have to re-process the resulting list in the caller.
No, we can't do that here. There are places where this function is called where that generator expression is inappropriate (see: tools/llvm-shlib/CMakeLists.txt).


================
Comment at: lib/Target/CMakeLists.txt:25
+# libraries being created.
+set_property(GLOBAL PROPERTY LLVM_TARGET_REGISTRATION_COMPLETE On)
----------------
bryant wrote:
> CC from IRC:
> 
> ```
> 9:12:17 < zq> beanz: beanz[m] crazy idea: if we're gonna require that target-depending components be scanned _after_ lib/Targets
> 19:12:29 < zq> then why even bother with those synonyms?
> 19:12:52 < zq> beanz: beanz[m] like ALLTARGETS and so forth
> 19:13:21 < zq> beanz: beanz[m] we could set those as special vars in lib/TArget/CMakeLists.txt right around where LLVM_TARGET_REGISTRATION_COMPLETE is set
> ```
There is value to the synonyms, but I have some ideas for how we can improve them. I think that shouldn't be part of this patch though.


https://reviews.llvm.org/D28869