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

[serge via Phabricator via llvm-commits]

serge-sans-paille marked 7 inline comments as done.
serge-sans-paille added a comment.

> I will have to try out the patch, e.g. on Windows. Please ping me if it takes to long.

Thanks  @Meinersbur



================
Comment at: CMakeLists.txt:1124
+
+# generate xfor file for extension handling
+get_property(LLVM_EXTENSIONS GLOBAL PROPERTY LLVM_EXTENSIONS)
----------------
mehdi_amini wrote:
> What is `xfor file` by the way?
> 
> Also can you expand on the documentation here? (what is the file containing, etc.)
I was thinking « X Macro » :-) https://en.wikipedia.org/wiki/X_Macro


================
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")
----------------
mehdi_amini wrote:
> Where is this property populated?
Fixed in register_llvm_extension


================
Comment at: cmake/modules/AddLLVM.cmake:811
+        if(TARGET ${tool})
+            set_property(TARGET ${tool} APPEND PROPERTY EXTENSIONS ${name} ${ARGN})
+        endif()
----------------
mehdi_amini wrote:
> 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})
Because it's illegal in cmake to call target_link_libraries in a different file, see https://cmake.org/cmake/help/v3.0/command/target_link_libraries.html (this has been changed in recent version of cmake though)


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

https://reviews.llvm.org/D61446