[PATCH] D157844: [llvm][CMake] Improve error message for unknown or experimental targets

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/CMakeLists.txt:924
-  list(FIND LLVM_EXPERIMENTAL_TARGETS_TO_BUILD ${t} idy)
-  # At this point, LLVMBUILDTOOL already checked all the targets passed in
-  # LLVM_TARGETS_TO_BUILD and LLVM_EXPERIMENTAL_TARGETS_TO_BUILD, so
----------------
DavidSpickett wrote:
> This is the only reference to LLVMBUILDTOOL in the monorepo. Perhaps it meant something once but it isn't doing anything now.
Yea, this is referring to the old LLVMBuild system that was used to manage the build dependency graph. It was removed, so the comment is out of date.


================
Comment at: llvm/CMakeLists.txt:930
+
+  if( idx_all_targets LESS 0 AND idx_experimental_to_build LESS 0)
+    if( idx_all_experimental_targets GREATER_EQUAL 0)
----------------
I find the CMake `if(… IN_LIST …)` syntax to be easier to read and follow (see: https://cmake.org/cmake/help/latest/command/if.html).


================
Comment at: llvm/CMakeLists.txt:935
+    else()
+      message(FATAL_ERROR "The target `${t}' is not a default target. It may be "
+        "experimental, if so it must be passed via "
----------------
nit: I don’t really love the “default” target phrasing. We don’t really have a good way of distinguishing the “all but experimental” target list.

Two alternate suggestions:
1) We could call them “known” because they are targets known to the build system.
2) We could call them “core tier” which would borrow language from the community support policy (see: https://www.llvm.org/docs/SupportPolicy.html).

I slightly prefer option 2.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157844/new/

https://reviews.llvm.org/D157844