cmake: Avoid leading space in LLVM_DEFINITIONS.

[chfast]

Summary

Unnecessary space at the beginning of LLVM_DEFINITIONS in cmake shared files can break projects that use the variable.

[chapuni]

I guess it would work. By the way, what is your issue?

I think LLVM_DEFINITIONS may be list-ized.

[chfast]

I'm getting a compiler command like clang++ -D -D_GNU_SOURCE.

[chfast]

Ping?

[john.brawn]

In D13432#259652, @chapuni wrote:

I guess it would work. By the way, what is your issue?

It appears that when there's a leading space in LLVM_DEFINITIONS that

add_definitions(${LLVM_DEFINITIONS}

works as expected, but

target_compile_definitions(example PUBLIC ${LLVM_DEFINITIONS})

gets you -D -DWHATEVER. Removing the leading space in LLVM_DEFINITIONS seems to make target_compile_definitions behave as expected.

[chfast]

Alternative solution is to keep LLVM_DEFINITIONS as a cmake lists (; separated values). And convert it into the flags string on demand. E.g. with a helper function like llvm_get_definitions.

[jsroemer]

In D13432#260571, @chfast wrote:

Alternative solution is to keep LLVM_DEFINITIONS as a cmake lists (; separated values). And convert it into the flags string on demand. E.g. with a helper function like llvm_get_definitions.

That should be the preferred solution, as this matches both the correct use of add_definitions and target_compile_definitions.

Also, have you considered replacing add_llvm_definitions by proper use of target_compile_definitions? That would make LLVM_DEFINITONS unnecessary.

[chfast]

I agree target_compile_definitions and others are super nice. But that transition would probably take some time and might break many things. I'm also not sure that information will be preserved by exported targets. However I can start working on that as a separated change after broader acceptance.

This change is a quick fix. Not very beautiful but helps in my case.

[jsroemer]

In D13432#260574, @chfast wrote:

I agree target_compile_definitions and others are super nice. But that transition would probably take some time and might break many things. I'm also not sure that information will be preserved by exported targets. However I can start working on that as a separated change after broader acceptance.

This change is a quick fix. Not very beautiful but helps in my case.

Does turning it back into a list break anything? If not the "quick fix" might be as easy as removing the quotes from the set line.

[chfast]

Changing LLVM_DEFINITIONS back to list needs also changes in other places and would probably break third party projects that use the variable.

[jsroemer]

Having had another look at how add_llvm_definitions is used, it is broken for target_compile_definitions anyways as LLVM_DEFINITIONS can contain compile options as well (something like -m32 would be converted to -D-m32, which is illegal, if passed to target_compile_definitions).

[beanz]

I haven't tested it yet, but I suspect something like this would work by treating LLVM_DEFINITONS as a list:

diff --git a/cmake/modules/AddLLVMDefinitions.cmake b/cmake/modules/AddLLVMDefinitions.cmake
index 33ac973..c4d3cc8 100644
--- a/cmake/modules/AddLLVMDefinitions.cmake
+++ b/cmake/modules/AddLLVMDefinitions.cmake
@@ -6,8 +6,6 @@
 
 macro(add_llvm_definitions)
   # We don't want no semicolons on LLVM_DEFINITIONS:
-  foreach(arg ${ARGN})
-    set(LLVM_DEFINITIONS "${LLVM_DEFINITIONS} ${arg}")
-  endforeach(arg)
+  list(APPEND LLVM_DEFINITIONS ${ARGN})
   add_definitions( ${ARGN} )
 endmacro(add_llvm_definitions)
diff --git a/cmake/modules/LLVMConfig.cmake.in b/cmake/modules/LLVMConfig.cmake.in
index 6855c44..a71d8d8 100644
--- a/cmake/modules/LLVMConfig.cmake.in
+++ b/cmake/modules/LLVMConfig.cmake.in
@@ -49,7 +49,7 @@ set(LLVM_LIBDIR_SUFFIX @LLVM_LIBDIR_SUFFIX@)
 
 set(LLVM_INCLUDE_DIRS "@LLVM_CONFIG_INCLUDE_DIRS@")
 set(LLVM_LIBRARY_DIRS "@LLVM_CONFIG_LIBRARY_DIRS@")
-set(LLVM_DEFINITIONS "@LLVM_DEFINITIONS@")
+set(LLVM_DEFINITIONS @LLVM_DEFINITIONS@)
 set(LLVM_CMAKE_DIR "@LLVM_CONFIG_CMAKE_DIR@")
 set(LLVM_TOOLS_BINARY_DIR "@LLVM_CONFIG_TOOLS_BINARY_DIR@")

[chfast]

That does not work. You will get set(LLVM_DEFINITIONS -D_GNU_SOURCE;-D__STDC_CONSTANT_MACROS;-D__STDC_FORMAT_MACROS;-D__STDC_LIMIT_MACROS)
and llvm-config --cppflags: -D_GNU_SOURCE;-D__STDC_CONSTANT_MACROS;-D__STDC_FORMAT_MACROS;-D__STDC_LIMIT_MACROS.

[chapuni]

For now, I think this may be fed to trunk as *quick fix*, as far as we shall be working on this area.

[chfast]

I will land the change today. Any objections? Someone else would like to send an approval?

llvm/trunk/cmake/modules/AddLLVMDefinitions.cmake

 # There is no clear way of keeping track of compiler command-line
 # options chosen via `add_definitions', so we need our own method for
 # using it on tools/llvm-config/CMakeLists.txt.
 
 # Beware that there is no implementation of remove_llvm_definitions.
 
 macro(add_llvm_definitions)
   # We don't want no semicolons on LLVM_DEFINITIONS:
   foreach(arg ${ARGN})
+    if(DEFINED LLVM_DEFINITIONS)
       set(LLVM_DEFINITIONS "${LLVM_DEFINITIONS} ${arg}")
+    else()
+      set(LLVM_DEFINITIONS ${arg})
+    endif()
   endforeach(arg)
   add_definitions( ${ARGN} )
 endmacro(add_llvm_definitions)