[PATCH] D47560: [CMake] Use find_package to find zlib

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: llvm/lib/Support/CMakeLists.txt:2
 set(system_libs)
-if ( LLVM_ENABLE_ZLIB AND HAVE_LIBZ )
+if ( HAVE_LIBZ )
   set(system_libs ${system_libs} ${ZLIB_LIBRARIES})
----------------
delcypher wrote:
> You shouldn't drop `LLVM_ENABLE_ZLIB` in the condition here, at least in the current implementation anyway. AFAIK both `LLVM_ENABLE_ZLIB` and `HAVE_LIBZ` are cache variables.
> 
> What could happen is that the user configures with  `LLVM_ENABLE_ZLIB` and ZLIB is found and used.
> Then at a later date with an existing build directory the user decides to set `LLVM_ENABLE_ZLIB` to `FALSE`.
> If that happens `HAVE_LIBZ` will still be `TRUE` causing the code to continue to use Zlib despite `LLVM_ENABLE_ZLIB` being
> set to `FALSE`.
Again. I think I confused `HAVE_LIBZ` with `ZLIB_FOUND` so your implementation is probably fine in the scenario I mentioned.


Repository:
  rL LLVM

https://reviews.llvm.org/D47560