[PATCH] D94387: Add new LLVMComponents CMake module.

[serge via Phabricator via llvm-commits]

serge-sans-paille added a comment.

First review round. I'm pretty excited by this work, thanks for handling this!

As a side note, I'd feel much more comfortable landing this patch if there was an actual use of it in the codebase, even for just one component. Other wise it can land and stay there, we have no way to ensure it works as expected. Maybe we already have a component that matches your model?



================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:124
+#
+#   * `${libname}_impl`: An object library containing the objects that should be
+#     contributed to the component.
----------------
I've see `${libname}_obj(s)` used a lot across cmakefiles to carry that meaning.


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:147
+  # Validate arguments.
+  if(ARG_ADD_TO_COMPONENT)
+    set(component_name ${ARG_ADD_TO_COMPONENT})
----------------
Do we want to normalize upper/lower case here?


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:165
+  set_target_properties(${_impl_target} PROPERTIES
+    LLVM_LIBRARY_IS_NEWCOMPONENT_LIB TRUE)
+
----------------
Out of curiosity, why `NEWCOMPONENT` and not `COMPONENT`


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:188
+    # Create the component.
+    llvm_component_create(${component_name})
+  endif()
----------------
I which situation can it already exist? And if it does  should we check it's not already *populated* ?


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:217
+      PRIVATE ${ARG_LINK_LIBS})
+  endif()
+
----------------
The two calls above are identical, and also similar to the calls on `${_impl_target}`. Maybe syndicate them in a function, or in a loop?


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:253
+  set(actual_shared_target ${component_shared_target})
+  if(NOT component_shared_target)
+    set(actual_shared_target ${component_static_target})
----------------
[nit] I'd rather use an `if() / else() ` here.


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:399
+# the component. The components system does nothing with this target but
+# ensures it is present and isolated from real library targets.
+function(llvm_component_get_props_target out_target name)
----------------
instead of providing this function, we could provide a `get_target_component_property(component_name...) that would make the  `_props` an implementation detail?


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:439
+  set_property(SOURCE ${_dummy_file} APPEND_STRING PROPERTY COMPILE_FLAGS
+      "-Wno-empty-translation-unit")
+  set(${out_name} ${_dummy_file} PARENT_SCOPE)
----------------
Portability issue: LLVM is supposed to compile with MSVC, and it doesn't support that flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94387