[llvm-commits] CMake patch

[Óscar Fuentes]

arrowdodger <6yearold at gmail.com> writes:

> Hello. I've sent my patch 2 days ago, but still no response.
>
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20110103/114729.html

Seems that gmane ate my response. Here it goes again:

arrowdodger <6yearold at gmail.com> writes:

> Posting first patch here as Óscar suggested in this thread
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-January/037209.html
> I've decided to start with simple stuff.
>
> Short summary:
>
> - Add ENABLE_CBE_PRINTF_A.
> - Fix ENABLE_PIC.
>     The problem was that ENABLE_PIC was set to 1 *after* generating
> config.h. I've moved if(ENABLE_PIC) from CMakeLists.txt into
> cmake/config-ix.cmake.
> - Add <ctype.h> check.
> - Add fmodf() call check.
>     This call located in math.h on Linux, FreeBSD, MacOS and Windows, so i
> decided, that it's safe to check it using just check_symbol_exists()
> command.
> - Add HAVE_INT64_T.
> - Fix HAVE_INT64_T, HAVE_UINT64_T and HAVE_U_INT64_T defines in
> config.h.cmake.
> - Remove CAN_DLOPEN_SELF.
>     This was only in config.h.cmake and not in config.h.in. Additionaly,
> there was none check for it.
>
> I want to mention, that all CMake code is slightly messed up: declarations
> are messed with logic,

What are declarations? In general, code related with one purpose should
be together. If this means putting declarations (whatever they are)
scattered all around, it is okay. If you are talking about something
else, please provide an example.

> there is no convention on naming variables
> (LLVM_ENABLE_<> and simply ENABLE_<>)

If there is an user-settable option that does not follow the LLVM_<>
naming convention, please report it. It is okay, but not required, to
nameve variables which are not user-settable as LLVM_<>.

> and other things. What do you think about cleaning this up?

Any cleaning up is great, but first I'll like to be sure about what you
are talking about.

> Index: cmake/config-ix.cmake
> ===================================================================
> --- cmake/config-ix.cmake	(revision 122744)
> +++ cmake/config-ix.cmake	(working copy)
> @@ -31,6 +31,7 @@
>  # include checks
>  check_include_file(argz.h HAVE_ARGZ_H)
>  check_include_file(assert.h HAVE_ASSERT_H)
> +check_include_file(ctype.h HAVE_CTYPE_H)
>  check_include_file(dirent.h HAVE_DIRENT_H)
>  check_include_file(dl.h HAVE_DL_H)
>  check_include_file(dld.h HAVE_DLD_H)
> @@ -92,6 +93,7 @@
>  check_symbol_exists(isnan math.h HAVE_ISNAN_IN_MATH_H)
>  check_symbol_exists(ceilf math.h HAVE_CEILF)
>  check_symbol_exists(floorf math.h HAVE_FLOORF)
> +check_symbol_exists(fmodf math.h HAVE_FMODF)
>  check_symbol_exists(nearbyintf math.h HAVE_NEARBYINTF)
>  check_symbol_exists(mallinfo malloc.h HAVE_MALLINFO)
>  check_symbol_exists(malloc_zone_statistics malloc/malloc.h
> @@ -135,6 +137,7 @@
>    set(headers ${headers} "stdint.h")
>  endif()
>  
> +check_type_exists(int64_t "${headers}" HAVE_INT64_T)
>  check_type_exists(uint64_t "${headers}" HAVE_UINT64_T)
>  check_type_exists(u_int64_t "${headers}" HAVE_U_INT64_T)
>  
> @@ -267,6 +270,24 @@
>    message(STATUS "Threads disabled.")
>  endif()
>  
> +#ENABLE_PIC value goes into config.h
> +set(ENABLE_PIC 0)
> +if( LLVM_ENABLE_PIC )
> + if( XCODE )
> +   # Xcode has -mdynamic-no-pic on by default, which overrides -fPIC. I don't
> +   # know how to disable this, so just force ENABLE_PIC off for now.
> +   message(STATUS "Warning: -fPIC not supported with Xcode.")
> + else( XCODE )
> +   if( SUPPORTS_FPIC_FLAG )
> +      message(STATUS "Building with -fPIC")
> +      add_llvm_definitions(-fPIC)
> +      set(ENABLE_PIC 1)
> +   else( SUPPORTS_FPIC_FLAG )
> +      message(STATUS "Warning: -fPIC not supported.")
> +   endif()
> + endif()
> +endif()

This is one example of code dispersion I don't like to
see. LLVM_ENABLE_PIC is declared in the top level CMakeLists.txt. Moving
away code that warns the user about his settings offers no gain. You are
right that include(config-ix) should be after that chunk of code,
though.

[snip]

> Index: CMakeLists.txt
> ===================================================================
> --- CMakeLists.txt	(revision 122744)
> +++ CMakeLists.txt	(working copy)
> @@ -101,6 +101,9 @@
>      CACHE STRING "Semicolon-separated list of targets to build, or \"all\".")
>  endif( MSVC )
>  
> +set(ENABLE_CBE_PRINTF_A 1 CACHE BOOL
> +  "Set to YES if CBE is enabled for printf %a output")
> +

Please use `option' and not `set' for  boolean user-settable variables.

>  set(CLANG_RESOURCE_DIR "" CACHE STRING
>    "Relative directory from the Clang binary to its resource files.")
>  
> @@ -195,26 +198,10 @@
>    endif(UNIX)
>  endif(WIN32)
>  
> -include(config-ix)
> -
>  option(LLVM_ENABLE_PIC "Build Position-Independent Code" ON)
> +set(ENABLE_TIMESTAMPS 1 CACHE BOOL "Enable embedding timestamp information in build")

Same.
  
> -set(ENABLE_PIC 0)
> -if( LLVM_ENABLE_PIC )
> - if( XCODE )
> -   # Xcode has -mdynamic-no-pic on by default, which overrides -fPIC. I don't
> -   # know how to disable this, so just force ENABLE_PIC off for now.
> -   message(STATUS "Warning: -fPIC not supported with Xcode.")
> - else( XCODE )
> -   if( SUPPORTS_FPIC_FLAG )
> -      message(STATUS "Building with -fPIC")
> -      add_llvm_definitions(-fPIC)
> -      set(ENABLE_PIC 1)
> -   else( SUPPORTS_FPIC_FLAG )
> -      message(STATUS "Warning: -fPIC not supported.")
> -   endif()
> - endif()
> -endif()
> +include(config-ix)

Overall looks good. Please return the if( LLVM_ENABLE_PIC ) chunk to the
toplevel CMakeLists.txt, replace those set(... BOOL ...) with option,
and commit if you have write permissions, or post again the patch if
not.

Thanks!