[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Wow that's much simpler!



================
Comment at: CMakeLists.txt:1124
+
+# generate xfor file for extension handling
+get_property(LLVM_EXTENSIONS GLOBAL PROPERTY LLVM_EXTENSIONS)
----------------
What is `xfor file` by the way?

Also can you expand on the documentation here? (what is the file containing, etc.)


================
Comment at: CMakeLists.txt:1125
+# generate xfor file for extension handling
+get_property(LLVM_EXTENSIONS GLOBAL PROPERTY LLVM_EXTENSIONS)
+file(WRITE "${CMAKE_BINARY_DIR}/include/llvm/Support/Extension.def.tmp" "//extension handlers\n")
----------------
Where is this property populated?


================
Comment at: cmake/modules/AddLLVM.cmake:811
+        if(TARGET ${tool})
+            set_property(TARGET ${tool} APPEND PROPERTY EXTENSIONS ${name} ${ARGN})
+        endif()
----------------
Can you add a comment explaining what you're doing here?
(it is easy to figure in the context of this diff, but it may not be obvious from the point of view of the reader out-of-context).


Also is there a convention for custom property names in CMake? You're using "EXTENSIONS" as a "magic" keyword here, I would use a "LLVM_" prefix (both to make it clear that this is a LLVM thing, and to avoid possible collision).
Since this is used for target linking, maybe make it part of the name as well? Something like "LLVM_EXTENSIONS_LINK"?  



================
Comment at: cmake/modules/AddLLVM.cmake:811
+        if(TARGET ${tool})
+            set_property(TARGET ${tool} APPEND PROPERTY EXTENSIONS ${name} ${ARGN})
+        endif()
----------------
mehdi_amini wrote:
> Can you add a comment explaining what you're doing here?
> (it is easy to figure in the context of this diff, but it may not be obvious from the point of view of the reader out-of-context).
> 
> 
> Also is there a convention for custom property names in CMake? You're using "EXTENSIONS" as a "magic" keyword here, I would use a "LLVM_" prefix (both to make it clear that this is a LLVM thing, and to avoid possible collision).
> Since this is used for target linking, maybe make it part of the name as well? Something like "LLVM_EXTENSIONS_LINK"?  
> 
If the target has to exist here and since you're using this to make it link a library, why are you going through properties in the first place? Why can't you do here: `target_link_libraries(${tool} PRIVATE ${name})


================
Comment at: tools/polly/CMakeLists.txt:212
+register_llvm_extension(polly)
+set_property(TARGET bugpoint APPEND PROPERTY EXTENSIONS LLVMTarget)
----------------
That looks like leftover experiments?


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

https://reviews.llvm.org/D61446