[PATCH] D94387: Add new LLVMComponents CMake module.

[Stella Laurenzo via Phabricator via llvm-commits]

stellaraccident added a comment.

PTAL - I ran out of time this week to make substantive progress on this, but I took a pass through the comments and cleaned up. When I next have time, I'll work on finishing the dev work on it to get everything working, then integrate it into the existing CMAKE flags and checkpoint to see where we are.



================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:147
+  # Validate arguments.
+  if(ARG_ADD_TO_COMPONENT)
+    set(component_name ${ARG_ADD_TO_COMPONENT})
----------------
serge-sans-paille wrote:
> Do we want to normalize upper/lower case here?
Ok - took a pass through and normalized.


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

```
  # TODO: Rename "NEWCOMPONENT" -> "COMPONENT" once migration is complete.
  # Just choosing a different name to avoid collisions with the existing
  # implementation until done.
```

I had issues early on with finding old code that used same named properties. I'll clean this up a bit later once I'm sure it is all gone.


================
Comment at: llvm/cmake/modules/LLVMComponents.cmake:188
+    # Create the component.
+    llvm_component_create(${component_name})
+  endif()
----------------
serge-sans-paille wrote:
> I which situation can it already exist? And if it does  should we check it's not already *populated* ?
For multi-library components (currently includes all targets but there are other cases in MLIR). The first one through defines it. Added a comment. Since this is also the code that populates it, not sure how much more verification we want to do... future us may appreciate a nice error here if someone collides in the namespace with a non-component library. Wdyt?


================
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)
----------------
serge-sans-paille wrote:
> instead of providing this function, we could provide a `get_target_component_property(component_name...) that would make the  `_props` an implementation detail?
Good idea. Made that change. Will probably also need to add a setter once I finish the porting. Currently this is used for SYSTEM_LIBS and I have that commented out.


================
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)
----------------
serge-sans-paille wrote:
> Portability issue: LLVM is supposed to compile with MSVC, and it doesn't support that flag.
Ah - I just ripped that off from somewhere while waiting to find where to shove a dummy.cpp file. Went looking and found one already there so updated everything to use it. It has a typedef in it so as to avoid triggering this warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94387