[PATCH] Document new LLVM CMake interface and emit warning when using llvm_map_components_to_libraries

[Brad King]

On 07/23/2014 06:49 AM, Dan Liew wrote:
> Here are my patches to document the LLVM CMake interface and also warn
> developers when using the old interface.

Thanks.  Good start.  Here are some comments:

> +  project(SimpleProject)

Precede this line with::

  cmake_minimum_required(VERSION 2.8.8)

since that is the minimum required by LLVM.  All CMake projects should
start with such a line so the example should show it.

> +  set(LLVM_DIR "" CACHE FILEPATH "Path to directory containing LLVMConfig.cmake")
> +  find_package(LLVM REQUIRED CONFIG)

The find_package command creates the LLVM_DIR cache entry automatically
so the set() line above is not needed.

> +  # Link against LLVM libraries
> +  target_link_libraries(simple-tool LLVMSupport LLVMCore LLVMIRReader)

IIUC applications should still use llvm_map_components_to_libnames
which maps LLVM components to a list of libraries but does not expand
dependencies.  For example:

 llvm_map_components_to_libnames(llvm_libs native option bitreader)
 target_link_libraries(simple-tool ${llvm_libs})

> +cmake manual for details). The ``LLVM_DIR`` variable can be set to hint to
> +``find_package(..)`` where to look for the ``LLVMConfig.cmake``.

LLVM_DIR is not just a hint.  It is the place that find_package
checks for an existing result and/or stores its new result.
Perhaps::

  It creates a ``LLVM_DIR`` cache entry to save the location where
  ``LLVMConfig.cmake`` is found or allow the user to specify the
  location.

> +This file is available in two different locations.

I think the install-tree location should be listed first since that
is what applications will typically use, especially when LLVM has
been installed by the package manager.  Also, this might be a good
location to recommend pointing CMAKE_PREFIX_PATH at the top of the
installation prefix (if it is not a system prefix) or build directory
when building an application.  Then find_package will be able to
locate LLVMConfig.cmake and set LLVM_DIR without the user having to
poke around to find share/llvm/cmake.

> +The ``LLVMConfig.cmake`` file sets various useful variables. Notable variables
> +include

Good.  I think the list of variables would be better formatted as
a definition list, and without the ":<type>" suffixes since they
are not cache entries.  For example::

  ``LLVM_CMAKE_DIR``
    The path to the directory containing LLVM CMake modules.

  ``LLVM_DEFINITIONS``
    A list of preprocessor defines that should be used with
    ``add_definitions()`` when building against LLVM.

> +  set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${LLVM_CMAKE_DIR}")

This can be::

  list(APPEND CMAKE_MODULE_PATH ${LLVM_CMAKE_DIR})

In the second patch:

> +  message(AUTHOR_WARNING "Using llvm_map_components_to_libraries() is deprecated. Use LLVM libraries as imported targets instead.")

This message may need to be updated based on the decision about the
above comment suggesting use of llvm_map_components_to_libnames.

-Brad