[PATCH] D18987: Add SVN version to libLLVMLTO

[Chris Bieneman via llvm-commits]

beanz accepted this revision.
beanz added a comment.

This all looks reasonable. As a follow-up it might be nice to refactor some of the related clang infrastructure to reduce duplication of functionality, but that shouldn't block landing this change.

A few minor comments inline.


================
Comment at: cmake/modules/VersionFromVCS.cmake:5
@@ -4,3 +4,3 @@
 
-function(add_version_info_from_vcs VERS)
+function(add_version_info_from_vcs VERS SOURCE_DIR)
   string(REPLACE "svn" "" result "${${VERS}}")
----------------
I assume you'll also have a corresponding clang patch to update the caller there, correct? Alternatively you could have the source_dir be an optional parameter and have it fall back to CMAKE_CURRENT_SOURCE_DIR if ARGN == 0.

================
Comment at: lib/LTO/CMakeLists.txt:22
@@ +21,3 @@
+# The VC revision include that we want to generate.
+set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/Revision.inc")
+
----------------
Can you name this something that identifies that it is just for libLTO (i.e. LTORevision.h)?


http://reviews.llvm.org/D18987