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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai 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}")
----------------
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.)


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:33
+if (DEFINED SOURCE_DIRS AND DEFINED NAMES)
+  list(LENGTH SOURCE_DIRS source_dirs_length)
+  list(LENGTH NAMES names_length)
----------------
mgorny wrote:
> To be honest, seeing all this extra logic to handle two lists, I kinda feel like a simpler solution would be to pass colon-separated sublist, e.g. `LLVM:directory;CLANG:directory`. But that's just a loose idea.
You'd need to verify the colon separation in that case, so I think it'd end up being a wash...

Another possibility would be to still have `NAMES` for specifying all projects, but then have <NAME>_SOURCE_DIR variables  to specify the source directories and have the script ensure all those variables are defined. In other words, if you passed `-DNAMES=LLVM;CLANG`, you'd also pass `-DLLVM_SOURCE_DIR=...` and `-DCLANG_SOURCE_DIR=...`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57063