[PATCH] D57063: [CMake] Unify scripts for generating VCS headers

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/CMakeLists.txt:756
+get_source_info(${CMAKE_CURRENT_SOURCE_DIR} revision repository)
+if (revision MATCHES "[0-9]+")
+  set(LLVM_RPM_SPEC_REVISION "r${revision}")
----------------
smeenai wrote:
> Super nit: I think it's more common in LLVM's CMake to *not* have the space between the if and the opening parenthesis, i.e. you'd have `if(...)` instead of `if (...)`. (I only noticed cos you were reformatting in a bunch of other places.)
I saw both styles in LLVM but never figured out which one is the official. Doing a quick grep, I found 975 for `if(...)` and 525 for `if (...)` so it seems like the version without space is indeed more common.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:22
+function(append_info name path)
+  #add_version_info_from_vcs(REVISION ${path})
+  get_source_info("${path}" revision repository)
----------------
mgorny wrote:
> Why is this commented out?
Leftover, this version was replaced by `get_source_info`.


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:58
+      if (EXISTS "${git_dir}/svn/refs")
+        execute_process(COMMAND ${GIT_EXECUTABLE} svn info
+          WORKING_DIRECTORY ${path}
----------------
mgorny wrote:
> Can't you reuse `get_source_info_git_svn` here?
I considered it but that macro does some duplicate work like finding Git and finding the working directory which seems unnecessary.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57063/new/

https://reviews.llvm.org/D57063