delcypher added inline comments.

================
Comment at: cmake/modules/FindZ3.cmake:3
+# in the find_path() and find_library() calls
+find_package(PkgConfig QUIET)
+PKG_CHECK_MODULES(PC_Z3 QUIET libz3)
----------------
@ddcc Seeing as you don't want to use the new upstream Z3 CMake package config 
files I'll try to review this.

Upstream Z3 does not come with pkg-config files for the native library so I'm 
wondering where you expect this to work. Does a Linux distro add their own 
`.pc` files?

It looks like you only use these paths as hints so this so this looks like 
it'll work even without the pkg-config files.


================
Comment at: cmake/modules/FindZ3.cmake:5
+PKG_CHECK_MODULES(PC_Z3 QUIET libz3)
+set(Z3_DEFINITIONS ${PC_LIBZ3_CFLAGS_OTHER})
+
----------------
@ddcc This CMake variable is set but never used. Also based on the name it 
suggests that they are compiler definitions rather than other compiler flags. 
Does `_CFLAGS_OTHER` have those semantics? It's unclear from 
https://cmake.org/cmake/help/v3.7/module/FindPkgConfig.html#command:pkg_check_modules
 what they are supposed to be.

To consume these flags you could add `target_compile_definitions()` and 
`target_compile_options()` to all users of Z3. A more elegant way of doing this 
is to create an imported library target (e.g. `z3::libz3`) and set compile 
definitions, compile options and include directories on this imported target 
with `INTERFACE` visibility so that these usage requirements implicitly 
propagate to all users of `z3::libz3`.


https://reviews.llvm.org/D28952



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to