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

[bryant via Phabricator via llvm-commits]

bryant 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})
----------------
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.


================
Comment at: cmake/modules/LLVM-Config.cmake:121
 
+  get_property(LLVM_TARGET_REGISTRATION_COMPLETE GLOBAL PROPERTY LLVM_TARGET_REGISTRATION_COMPLETE)
+
----------------
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.


================
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)
----------------
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)
```


================
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()
----------------
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?


================
Comment at: cmake/modules/LLVM-Config.cmake:248
+          # that this is valid here.
+          list(APPEND expanded_components LLVM${c})
         endif()
----------------
`list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>")` ? Then we won't have to re-process the resulting list in the caller.


================
Comment at: lib/Target/CMakeLists.txt:25
+# libraries being created.
+set_property(GLOBAL PROPERTY LLVM_TARGET_REGISTRATION_COMPLETE On)
----------------
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
```


https://reviews.llvm.org/D28869